Conversation
- Upgrade rxjs dependency to ^7.8.1 - Replace deprecated publishReplay/refCount with shareReplay in utility.service.ts 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Convert HighlightPipe, SafeHtmlPipe, TranslatePipe to standalone - Delete ExtrasModule (no longer needed) - Update AppModule, BrowserHPOModule, SearchModule to import standalone pipes - Fix test files to use standalone pipes in imports instead of declarations 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Components converted to standalone with specific Material imports: - FooterComponent (RouterModule, MatButtonModule) - NoPageFoundComponent (CommonModule, RouterModule, MatButtonModule) - FloatingFeedbackComponent (RouterModule, MatButtonModule, MatIconModule, MatTooltipModule) - AboutComponent (RouterModule, MatButtonModule, MatIconModule) - FeedbackComponent - All resource components (license, citation, contact, disclaimer, funding, faq) - All tools components (phenomizer, external, exomiser, genomiser, hpobrowser, phenogramviz, loinc) - All data components (api-doc, annotations-download, ontology-download, translation, indigenous, layperson) Updated modules to import standalone components instead of declaring them. Updated all test files to use imports array for standalone components. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Convert remaining NgModule-declared components to standalone: - AppComponent, NavbarComponent - HomeComponent, NewsComponent - SearchComponent - TermComponent, DiseaseComponent, GeneComponent - SearchResultsComponent, ProfileSearchComponent - DialogExcelDownloadComponent, PublicationsComponent Each component now declares its own specific Material imports instead of relying on GlobalMaterialModules. Module files updated to import standalone components rather than declare them. Test files updated to use TestBed.configureTestingModule with components in imports array instead of declarations. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Major changes: - Converted from platformBrowserDynamic().bootstrapModule() to bootstrapApplication() with provideRouter/provideHttpClient/provideAnimations - Created new .routes.ts files exporting route arrays for all feature areas - Deleted all NgModule files (app.module, browser.module, static.module, data.module, resources.module, tools.module, search.module) - Deleted all *-routing.module.ts files in favor of .routes.ts - Deleted GlobalMaterialModules - each component now has its own Material imports - Updated test files to remove GlobalMaterialModules dependencies The app now uses pure standalone architecture with no NgModules. Bundle size reduced by ~18KB (from 1.21MB to 991KB initial). 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
This PR migrates the Angular application from NgModules to standalone components and upgrades RxJS from version 6.5.4 to 7.8.1. The migration involves converting all components to standalone, replacing module-based routing with route files, and updating deprecated RxJS operators.
Key Changes
- Migration to standalone components architecture across the entire application
- RxJS upgrade from 6.5.4 to 7.8.1 with operator replacements (
publishReplay/refCount→shareReplay) - Module-based routing replaced with route configuration files
- Addition of a new feedback feature with floating feedback button
Reviewed changes
Copilot reviewed 106 out of 109 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| src/main.ts | Replaced module bootstrapping with standalone component bootstrapping using bootstrapApplication |
| src/app/app.component.ts | Converted to standalone component with direct imports |
| src/app/app.routes.ts | New routing configuration replacing app-routing.module.ts |
| src/app/static/**/*.component.ts | Converted all static page components to standalone |
| src/app/browser/pages/**/*.component.ts | Converted all browser page components to standalone |
| src/app/shared/**/*.component.ts | Converted shared components (navbar, footer, search) to standalone |
| src/app/shared/pipes/*.pipe.ts | Added standalone: true to all pipes |
| src/app/shared/utility/utility.service.ts | Updated RxJS operators from publishReplay/refCount to shareReplay |
| src/app/shared/news/news.service.ts | Updated RxJS operators and changed to root-provided service |
| **/*.spec.ts | Updated test configurations to import standalone components |
| package.json | Updated RxJS to 7.8.1 and various dev dependencies |
| .eslintrc.json | Updated ESLint configuration and added accessibility rules |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| Phenotypes - {{terms.length}} of {{termsCount}} displayed | ||
| </div> | ||
| <div class="result-list"> | ||
| <div class="result-list flex flex-column"> |
There was a problem hiding this comment.
The CSS classes 'flex' and 'flex-column' appear to be Tailwind utility classes, but this component's stylesheet is using SCSS. These classes should either be defined in the component's SCSS file or the component should consistently use one styling approach.
| <div class="id">{{item.id}}</div> | ||
| <br> |
There was a problem hiding this comment.
Using <br> tags for layout is an anti-pattern. The layout spacing should be controlled through CSS (margin/padding) in the component's SCSS file for better maintainability.
| <div class="id">{{item.id}}</div> | ||
| <br> |
There was a problem hiding this comment.
Using <br> tags for layout is an anti-pattern. The layout spacing should be controlled through CSS (margin/padding) in the component's SCSS file for better maintainability.
| styleUrls: ['./feedback.component.scss'] | ||
| }) | ||
| export class FeedbackComponent { | ||
| private formUrl = 'https://docs.google.com/forms/d/e/1FAIpQLSfID5F6is5J7GSJUlFzIYZ7FOsXEebjLjLZwXhLdFPELVBVMQ/viewform?usp=dialog'; |
There was a problem hiding this comment.
The Google Forms URL should be extracted to an environment configuration file rather than hardcoded in the component. This allows for easier configuration between development, staging, and production environments.
There was a problem hiding this comment.
@copilot open a new pull request to apply changes based on this feedback
| marginheight="0" | ||
| marginwidth="0"> |
There was a problem hiding this comment.
The marginheight and marginwidth attributes are deprecated in HTML5. These should be removed or replaced with CSS padding/margin styles if needed.
| marginheight="0" | |
| marginwidth="0"> | |
| style="margin: 0; padding: 0;"> |
| "eslint": "^8.57.0", | ||
| "@typescript-eslint/eslint-plugin": "7.11.0", | ||
| "@typescript-eslint/parser": "7.11.0", | ||
| "baseline-browser-mapping": "^2.9.11", |
There was a problem hiding this comment.
The baseline-browser-mapping package appears to be a new devDependency added in this PR. However, there's no visible usage of this package in the reviewed code changes. If it's not being used, it should be removed to avoid unnecessary dependencies.
| "baseline-browser-mapping": "^2.9.11", |
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: iimpulse <7397665+iimpulse@users.noreply.github.com>
Extract feedback form URL to environment configuration
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 122 out of 125 changed files in this pull request and generated 8 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // eslint-disable-next-line @typescript-eslint/no-var-requires | ||
| const packageJson = require('../../package.json'); |
There was a problem hiding this comment.
require('../../package.json') in an Angular environment file can break or complicate ESM/bundler builds (and usually isn’t tree-shakeable). Prefer a JSON import (with resolveJsonModule), or inject/version-stamp at build time (e.g., file replacement, define plugin, or a generated version.ts), and remove the inline eslint disable.
| production: false, | ||
|
|
||
| VERSION: require('../../package.json').version, | ||
| VERSION: packageJson.version, |
There was a problem hiding this comment.
require('../../package.json') in an Angular environment file can break or complicate ESM/bundler builds (and usually isn’t tree-shakeable). Prefer a JSON import (with resolveJsonModule), or inject/version-stamp at build time (e.g., file replacement, define plugin, or a generated version.ts), and remove the inline eslint disable.
| terms: this.ontologyService.search(query, limit).pipe( | ||
| catchError((e) => { | ||
| console.error(e); | ||
| return of<OntologySearchResponse>({ terms: [] }); |
There was a problem hiding this comment.
The catchError fallback for terms returns { terms: [] }, but callers previously relied on totalCount (and your UI still displays “x of y”). If OntologySearchResponse includes totalCount, return a complete fallback (e.g., { terms: [], totalCount: 0 }) to prevent undefined totals and inconsistent UI behavior.
| return of<OntologySearchResponse>({ terms: [] }); | |
| return of<OntologySearchResponse>({ terms: [], totalCount: 0 }); |
| this.diseases = diseases.results; | ||
| this.genes = genes.results; | ||
| this.termsCount = terms.totalCount; | ||
| this.termsCount = terms.terms.length; |
There was a problem hiding this comment.
termsCount is assigned terms.terms.length, which makes the “displayed of total” UI always show the same value for phenotypes. If the API provides a total (previous code used terms.totalCount), keep and display that value instead.
| this.termsCount = terms.terms.length; | |
| this.termsCount = terms.totalCount; |
| // lookahead -> lookbehind not suitable for production yet. | ||
| // Limited implementation to favor ending strings | ||
| const regex = new RegExp(subHighlight[x] + '(?!\#)', 'gi'); | ||
| const regex = new RegExp(subHighlight[x] + '(?!#)', 'gi'); |
There was a problem hiding this comment.
This builds a RegExp directly from user-provided input without escaping special characters. Queries containing regex metacharacters (e.g., (, [, \) can throw at runtime, and certain patterns can cause performance issues (regex backtracking). Escape the query tokens before constructing the regex (e.g., replace /[.*+?^${}()|[\]\\]/g), and consider bounding token length.
| catLabel: string; | ||
| annotationCount: number; | ||
| termSource: any; | ||
| termSource: MatTableDataSource<{frequency: string, onset: string, sources: string[]}>; |
There was a problem hiding this comment.
models.ts now depends on Angular Material (MatTableDataSource) via TermCategory, which couples domain models to a UI framework and can make reuse/testing harder. Prefer keeping models framework-agnostic (e.g., termSourceRows: { frequency: string; onset: string; sources: string[] }[]) and construct MatTableDataSource inside the relevant component instead.
| </div> | ||
| <div class="result-list flex flex-column"> | ||
| <div class="result" (click)="closeDropDown()" routerLink="/browse/term/{{item.id}}" | ||
| <div class="result" (click)="closeDropDown()" (keydown.enter)="closeDropDown()" tabindex="0" routerLink="/browse/term/{{item.id}}" |
There was a problem hiding this comment.
Using string interpolation inside routerLink attributes is brittle. Prefer property binding so route parameters are composed safely and consistently, e.g. [routerLink]="['/browse/term', item.id]" (same applies to disease/gene links below).
| <a [href]="otherReleases" target="_blank">Older Versions</a> | ||
| </div> | ||
| <div class="download-banner-wrapper mat-elevation-z4 flex flex-row" (click)="utilityService.downloadFile(oboUrl)"> | ||
| <div class="download-banner-wrapper mat-elevation-z4 flex flex-row" (click)="utilityService.downloadFile(oboUrl)" (keydown.enter)="utilityService.downloadFile(oboUrl)" tabindex="0" role="button"> |
There was a problem hiding this comment.
Elements with role="button" should generally support both Enter and Space activation. Add a Space key handler (and prevent default scrolling) or use a semantic <button> element styled to match; this will also align better with the enabled template accessibility lint rules.
Uh oh!
There was an error while loading. Please reload this page.