feat: trs #401
feat: trs #401
Conversation
|
Reviewer's Guide by SourceryThis pull request introduces a new feature for interacting with the GA4GH Tool Registry Service (TRS) API by implementing a new web component using LitElement. It also includes various configuration and setup files for the new package, along with some modifications to existing files to accommodate the new feature. No diagrams generated as the changes look simple and do not need a visual representation. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @anuragxxd - I've reviewed your changes and found some issues that need to be addressed.
Blocking issues:
- Hardcoded base URL found in HTML file. (link)
Overall Comments:
- Consider renaming the PR title to be more descriptive of the changes made, as 'feat: trs' is quite vague.
- The PR description is missing a summary of the changes and the issue it resolves; consider adding this information for better context.
Here's what I looked at during the review
- 🟡 General issues: 1 issue found
- 🔴 Security: 1 blocking issue
- 🟢 Testing: all looks good
- 🟡 Complexity: 1 issue found
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| }); | ||
|
|
||
| const response = await fetch(url); | ||
| if (!response.ok) { |
There was a problem hiding this comment.
suggestion: Consider refactoring error handling for fetch responses.
Since similar error handling logic is repeated across API methods, extracting a helper function for handling response errors could reduce redundancy and improve maintainability.
Suggested implementation:
async function handleResponse(response: Response): Promise<Response> {
if (!response.ok) {
throw new Error(`Failed to fetch: ${response.statusText}`);
}
return response;
}
// Add filter parameters const rawResponse = await fetch(url);
const response = await handleResponse(rawResponse);Depending on your codebase, you may want to reuse handleResponse in other API methods in this file. Make sure to update all similar inline error handling blocks to call handleResponse and adjust variable names accordingly.
| } | ||
| } | ||
|
|
||
| private async _handleExpandItem(event: CustomEvent): Promise<void> { |
There was a problem hiding this comment.
issue (complexity): Consider refactoring complex methods into smaller helper functions.
Consider extracting repeated or multi-step logic into helper functions. For example, you can refactor _handleExpandItem by moving the creation and rendering of the details component into its own helper, which would reduce nesting and clarify responsibilities.
Before:
private async _handleExpandItem(event: CustomEvent): Promise<void> {
const eccUtilsDesignCollection = this.getCollectionElement();
if (!eccUtilsDesignCollection) return;
const { target, detail } = event;
if (!target || !(target instanceof HTMLElement)) {
console.error("Target is null or not an HTMLElement");
return;
}
const { key } = detail;
const existingContent = target.querySelector(`[slot='${key}']`);
if (existingContent) {
console.log("Content already exists in slot:", key);
return;
}
if (!this.api) return;
try {
const toolData = this.tools.find((tool: any) => tool.id === key);
if (!toolData) {
console.error(`Failed to find tool with ID: ${key}`);
return;
}
this.cache.set(key, toolData);
const child = document.createElement("div");
child.setAttribute("slot", key);
target.appendChild(child);
const detailsComponent = document.createElement("ecc-utils-design-details");
detailsComponent.classList.add("details");
detailsComponent.id = key;
detailsComponent.data = toolData;
detailsComponent.fields = ECCClientGa4ghTrsTools.defaultFields;
child.appendChild(detailsComponent);
console.log("Added details component for:", key);
} catch (error) {
console.error(`Failed to process data for tool: ${key}`, error);
}
}After:
Split the creation of the details component and its container into helper functions.
private createDetailsContainer(key: string): HTMLDivElement {
const container = document.createElement("div");
container.setAttribute("slot", key);
return container;
}
private createDetailsComponent(key: string, toolData: any): HTMLElement {
const detailsComponent = document.createElement("ecc-utils-design-details");
detailsComponent.classList.add("details");
detailsComponent.id = key;
detailsComponent.data = toolData;
detailsComponent.fields = ECCClientGa4ghTrsTools.defaultFields;
return detailsComponent;
}
private async _handleExpandItem(event: CustomEvent): Promise<void> {
const collection = this.getCollectionElement();
if (!collection) return;
const { target, detail } = event;
if (!target || !(target instanceof HTMLElement)) {
console.error("Target is null or not an HTMLElement");
return;
}
const { key } = detail;
if (target.querySelector(`[slot='${key}']`)) {
console.log("Content already exists in slot:", key);
return;
}
if (!this.api) return;
try {
const toolData = this.tools.find((tool: any) => tool.id === key);
if (!toolData) {
console.error(`Failed to find tool with ID: ${key}`);
return;
}
this.cache.set(key, toolData);
const container = this.createDetailsContainer(key);
target.appendChild(container);
const detailsComponent = this.createDetailsComponent(key, toolData);
container.appendChild(detailsComponent);
console.log("Added details component for:", key);
} catch (error) {
console.error(`Failed to process data for tool: ${key}`, error);
}
}Similar modularization for complex inline conditional logic in updateFilters can be applied. For instance, separate the search filter configuration and tool class filter creation into helper functions:
private getSearchFilters(): FilterProp[] {
return [
{
key: "search",
type: "search",
placeholder: "Search by name",
},
];
}
private getAdditionalFilters(): FilterProp[] {
return [
{ key: "id", type: "search", placeholder: "Filter by Tool ID" },
{ key: "alias", type: "search", placeholder: "Filter by Alias" },
{ key: "registry", type: "search", placeholder: "Filter by Registry" },
{ key: "organization", type: "search", placeholder: "Filter by Organization" },
{ key: "name", type: "search", placeholder: "Filter by Name" },
{ key: "description", type: "search", placeholder: "Filter by Description" },
{ key: "author", type: "search", placeholder: "Filter by Author" },
];
}
private getToolClassFilter(): FilterProp | null {
if (this.toolClasses.length === 0) return null;
this.toolClassMap = Object.fromEntries(this.toolClasses.map(tc => [tc.name, tc.id]));
const toolClassOptions = this.toolClasses.map(tc => tc.name);
return {
key: "toolClass",
type: "select",
options: toolClassOptions,
placeholder: "Filter by tool class",
selectConfig: { multiple: false },
};
}
private updateFilters(): void {
let filters: FilterProp[] = [];
if (this.search) {
filters = filters.concat(this.getSearchFilters());
if (this.filter) {
filters = filters.concat(this.getAdditionalFilters());
}
}
if (this.filter) {
const toolClassFilter = this.getToolClassFilter();
if (toolClassFilter) {
filters.push(toolClassFilter);
}
}
this.filters = filters;
}These changes keep functionality intact while reducing the complexity of methods, improving readability and maintainability.
| @@ -14,7 +14,7 @@ | |||
| <body> | |||
| <div style="display: flex; justify-content: center; flex-direction: column"> | |||
| <ecc-client-ga4gh-trs | |||
| baseUrl="https://trs-filer-test.rahtiapp.fi/ga4gh/trs/v2" | |||
| baseUrl="https://dockstore.org/api/api/ga4gh/v2" | |||
There was a problem hiding this comment.
🚨 issue (security): Hardcoded base URL found in HTML file.
Consider using an environment variable or configuration file to manage the base URL for different environments (e.g., development, testing, production) to enhance security and flexibility.
|
|
||
| protected updated(changedProperties: Map<PropertyKey, unknown>): void { | ||
| const eccUtilsDesignCollection = this.getCollectionElement(); | ||
| if (!eccUtilsDesignCollection) return; |
There was a problem hiding this comment.
suggestion (code-quality): Use block braces for ifs, whiles, etc. (use-braces)
| if (!eccUtilsDesignCollection) return; | |
| if (!eccUtilsDesignCollection) { |
Explanation
It is recommended to always use braces and create explicit statement blocks.Using the allowed syntax to just write a single statement can lead to very confusing
situations, especially where subsequently a developer might add another statement
while forgetting to add the braces (meaning that this wouldn't be included in the condition).
| } | ||
|
|
||
| private async loadToolClasses(): Promise<void> { | ||
| if (!this.api) return; |
There was a problem hiding this comment.
suggestion (code-quality): Use block braces for ifs, whiles, etc. (use-braces)
| if (!this.api) return; | |
| if (!this.api) { |
Explanation
It is recommended to always use braces and create explicit statement blocks.Using the allowed syntax to just write a single statement can lead to very confusing
situations, especially where subsequently a developer might add another statement
while forgetting to add the braces (meaning that this wouldn't be included in the condition).
| } | ||
|
|
||
| private async loadData(): Promise<void> { | ||
| if (!this.api) return; |
There was a problem hiding this comment.
suggestion (code-quality): Use block braces for ifs, whiles, etc. (use-braces)
| if (!this.api) return; | |
| if (!this.api) { |
Explanation
It is recommended to always use braces and create explicit statement blocks.Using the allowed syntax to just write a single statement can lead to very confusing
situations, especially where subsequently a developer might add another statement
while forgetting to add the braces (meaning that this wouldn't be included in the condition).
| if (eccUtilsDesignCollection) { | ||
| // Don't set totalItems to enable dynamic pagination | ||
| // This will allow the collection to show "..." and enable next page button | ||
| // as long as there are items returned | ||
|
|
||
| // Update UI based on returned items | ||
| if (this.items.length === 0 && this.currentPage > 1) { | ||
| // If we get no results and we're not on the first page, go back a page | ||
| this.currentPage--; | ||
| this.loadData(); | ||
| return; | ||
| } | ||
| } |
There was a problem hiding this comment.
suggestion (code-quality): Merge nested if conditions (merge-nested-ifs)
| if (eccUtilsDesignCollection) { | |
| // Don't set totalItems to enable dynamic pagination | |
| // This will allow the collection to show "..." and enable next page button | |
| // as long as there are items returned | |
| // Update UI based on returned items | |
| if (this.items.length === 0 && this.currentPage > 1) { | |
| // If we get no results and we're not on the first page, go back a page | |
| this.currentPage--; | |
| this.loadData(); | |
| return; | |
| } | |
| } | |
| if (eccUtilsDesignCollection && (this.items.length === 0 && this.currentPage > 1)) { | |
| this.currentPage--; | |
| this.loadData(); | |
| return; | |
| } | |
Explanation
Reading deeply nested conditional code is confusing, since you have to keep track of whichconditions relate to which levels. We therefore strive to reduce nesting where
possible, and the situation where two
if conditions can be combined usingand is an easy win.
|
|
||
| private async _handleExpandItem(event: CustomEvent): Promise<void> { | ||
| const eccUtilsDesignCollection = this.getCollectionElement(); | ||
| if (!eccUtilsDesignCollection) return; |
There was a problem hiding this comment.
suggestion (code-quality): Use block braces for ifs, whiles, etc. (use-braces)
| if (!eccUtilsDesignCollection) return; | |
| if (!eccUtilsDesignCollection) { |
Explanation
It is recommended to always use braces and create explicit statement blocks.Using the allowed syntax to just write a single statement can lead to very confusing
situations, especially where subsequently a developer might add another statement
while forgetting to add the braces (meaning that this wouldn't be included in the condition).
| return; // Content already exists, no need to add it again | ||
| } | ||
|
|
||
| if (!this.api) return; |
There was a problem hiding this comment.
suggestion (code-quality): Use block braces for ifs, whiles, etc. (use-braces)
| if (!this.api) return; | |
| if (!this.api) { |
Explanation
It is recommended to always use braces and create explicit statement blocks.Using the allowed syntax to just write a single statement can lead to very confusing
situations, especially where subsequently a developer might add another statement
while forgetting to add the braces (meaning that this wouldn't be included in the condition).
Description
Fixes #(issue)
Checklist
Comments
Summary by Sourcery
Create a new TRS (Tool Registry Service) web component with enhanced functionality for browsing and filtering tools from GA4GH TRS APIs
New Features:
Enhancements:
Chores: