fix: bug fix for samplesheet, page navigation, and run file management #635
Conversation
k-florek
changed the title
There was a problem hiding this comment.
Pull Request Overview
This PR fixes bugs in samplesheet handling, table pagination, and file explorer navigation. The changes address incorrect page boundary calculations and directory navigation issues when multiple directories share the same name but have different paths.
Key Changes:
- Fixed pagination logic to calculate total pages based on total rows instead of rows per page
- Updated file tree navigation to handle directories with identical names in different paths
- Improved type handling for file tree children to support both intermediate and final data structures
Reviewed Changes
Copilot reviewed 2 out of 3 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| packages/front-end/src/app/components/EGTable.vue | Fixed page navigation boundary check to use total pages instead of rows per page |
| packages/front-end/src/app/components/EGFileExplorer.vue | Enhanced directory navigation to support same-named directories in different paths and improved type safety |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| let currentChildren = rootDirChildren; | ||
| // rootDirChildren can be a MapType or an array; treat it as any[] for navigation | ||
| let currentChildren = rootDirChildren as any; |
There was a problem hiding this comment.
Using 'as any' bypasses TypeScript's type safety. Consider using a type guard or union type (FileTreeNode[] | MapType) to maintain type checking while handling both possible types of rootDirChildren.
| const resultsChild = (currentChildren as any[]).find( | ||
| (node: any) => node.type === 'directory' && node.name === step, | ||
| ); |
There was a problem hiding this comment.
Multiple 'any' type assertions reduce type safety. Since children can be FileTreeNode[] or MapType, create a helper function to normalize MapType to FileTreeNode[] for consistent array operations.
| const filteredItems = computed(() => { | ||
| const query = searchQuery.value.toLowerCase(); | ||
| return currentItems.value.filter((item: MapType) => item?.name.toLowerCase().includes(query)); | ||
| return currentItems.value.filter((item: any) => (item?.name || '').toLowerCase().includes(query)); |
There was a problem hiding this comment.
The 'any' type assertion and defensive '|| ""' check suggest unclear typing. Consider defining a proper type for currentItems or adding a type guard to ensure items have the expected structure.
| return currentItems.value.filter((item: any) => (item?.name || '').toLowerCase().includes(query)); | |
| return currentItems.value.filter( | |
| (item): item is FileTreeNode & { class?: string } => | |
| typeof item.name === 'string' && item.name.toLowerCase().includes(query) | |
| ); |
| // navigating into directories that share the same name but live under different paths. | ||
| const last = currentPath.value[currentPath.value.length - 1]; | ||
| if (last === dir) return; | ||
| currentPath.value.push(dir as any); |
There was a problem hiding this comment.
The 'as any' cast here appears unnecessary since dir is already typed as FileTreeNode. If currentPath has a different type requirement, consider updating its type definition instead of using type assertions.
| currentPath.value.push(dir as any); | |
| currentPath.value.push(dir); |
marvinsumali
left a comment
marvinsumali
left a comment
There was a problem hiding this comment.
Zig's happy with the changes.
Note that File Manager changes can't be fully tested at our end at this point.
|
@k-florek , your PR looks good. Just as a note, we try to avoid the use of casting to Copilot's added some PR comments / suggestions to help avoid the use of |
|
I've updated the type to use FileTreeNode or MapType instead of any. I am not sure I can follow everything that is happening here but I've had this running for a week internally and everything seems to be working. I also added a new feature that auto populates a runname parameter with the run name provided by the user when starting the workflow. HealthOmics generates a random runname during runtime and we found that output files weren't matching the user's specified run name. Adding this parameter to the workflow allows the user to specify the runname once at the start of the workflow, which is then auto populated on the parameters page. This allows the pipeline to use the runname parameter to align the output files with the user entered runname. |
…ctories in the breadcrumb
… for the pipeline
4 participants
BugFix for samplesheet, page navigation, and run file management
Type of Change*
Description
I removed spaces from the samplesheet header and file paths, I also changed how EGTable component calculated the total number of pages and page navigation. With AI assistance I was able to alter the EGFileExplorer component, specifically the FileTreeNode and change it from a MapType to a MapType or FileTreeNode[] then altered the openDirectory function to allow navigation into directories that share the same name as another directory but a different path.
Testing*
I deployed this in our dev and prod environments and found the issues resolved with no other issues.
Impact
This PR improves user experience by fixing bugs
Additional Information
None
Checklist*