0.1.0 Technical Code Review

This ticket documents the technical code review performed against tag 0.1.0 of the nshmp-apps project.

assigned to @emartinez

• package.json and package-lock.json files should be updated to latest NPM format.
  • currently npm ci triggers an upgrade on the files
• .npmrc should be updated to remove reference to GITLAB_TOKEN
  • may require dependent projects to be publicly accessible first, understandable, but we should not require users have accounts/tokens in order to build project
• @types/d3-array should be a dev-dependency, not a dependency

package-lock.json updated in !107 (merged)

Moved @types/d3-array to dev dependencies, !109 (merged)

In general, documentation commands should use full-length version of flags, for example:

# Bad
npm i
ng g c
# Better
npm install
ng generate component

Specific to npm i, probably what is intended is npm ci

Updated documentation, !109 (merged)

Dependencies should be updated.

• Angular 12
• Node 16 (or Node 14 if limited by lambda runtimes)
• NPM dependencies should move to latest when possible
  • Try to reduce vulnerabilities in source code

Update to Angular 13, !138 (merged)

Building production (npm run build:prod) results in the following warnings:

Warning: C:/Users/.../nshmp-apps/projects/nshmp-lib/src/lib/components/error/error.component.scss exceeded maximum budget. Budget 6.00 kB was not met by 72.71 kB with a total of 78.71 kB.

Warning: C:\Users\...\nshmp-apps\projects\nshmp-apps\src\app\hazard\static\state\app.state.ts depends on '@ghsc/nshmp-utils'. CommonJS or AMD dependencies can cause optimization bailouts.
For more info see: https://angular.io/guide/build#configuring-commonjs-dependencies

Warning: initial exceeded maximum budget. Budget 3.00 MB was not met by 2.52 MB with a total of 5.52 MB.

Consider tree-shaking to optimize bundle sizes.

Fixed bundle size, !140 (merged)

Running ng lint results in one warning:

Linting "nshmp-apps"...

C:\Users\...\nshmp-apps\projects\nshmp-apps\src\app\source-models\data\state\app.effects.ts
  5:49  warning  'LngLatBoundsLike' is defined but never used  @typescript-eslint/no-unused-vars

✖ 1 problem (0 errors, 1 warning)

Lint warnings found in the listed files.

Also, why do we have gts lint and ng lint both on this project. Maybe pick one and go with that.

Remove gts, only using ng lint.

ng lint results in no errors or warnings.

Resolved by !139 (merged)

Use of any in typescript is discouraged.

Suggest adding no implicit any and no explicit any to compilers/linters to identify this. Variables, method arguments, and method return values should all be typed whenever possible.

Explicit any usage resolved, !272 (merged)

Implicit any resolved by !273 (merged)

The Google Typescript Styleguide (gts) is installed, but it only seems to be used by Prettier. Consider also using this in TSConfig and ESLint. Resolve any problems that arise.

Removed gts, !139 (merged)

Prefer to not use pipeline templates from external projects. If this is done, prefer to use a tagged, immutable ref rather than main.

Removed importing external projects, !117 (merged)

Pipeline improvements

• Include testing (unit and e2e) jobs in pipeline
• Consider adding - needs: [] to Build Image so this process can get started immediately, it is a long process.
• Include some security checks
  • npm audit
  • penetration tests (owasp)
  • static code analysis (trivy)

• Unit tests added: !141 (merged)
• Cypress e2e tests added: !142 (merged), !143 (merged), !148 (merged)

• Trivy analysis: !149 (merged)
• OWASP zap tests: !149 (merged)

Consider improving application performance. A lighthouse score of 75 or better should be a target. Application did not function properly with npm run start:prod, so these results were generated against npm run start. Current report snippet...

Fixed npm run start:prod, !150 (merged), !151 (merged)

Application is generally difficult to use on mobile devices.

• Maybe show single screen (input, plot, settings) as a toggle
• Plots shrink but do not grow as panels are toggled
• We are allowed to toggle off all panels and are left with a blank white screen
• Entering latitude and longitude generally results in a console error:

Uncaught DOMException: Failed to execute 'setSelectionRange' on 'HTMLInputElement': The input element's type ('number') does not support selection

• Autogenerated swagger pages are unusable

App controls fixed in nshmp-template!44 (merged)

Can no longer toggle off all panels.

Mobile improvements in !299 (merged)

Consider adding a jest.config.js configuration file for spec files, example template:

module.exports = {
  globals: {
    'ts-jest': {
      tsconfig: '<rootDir>/tsconfig.spec.json',
      stringifyContentPathRegex: '\\.html$',
      astTransformers: ['jest-preset-angular/InlineHtmlStripStylesTransformer'],
      diagnostics: false
    },
    Notification: true
  },
  name: 'utils',
  preset: 'jest-preset-angular',
  reporters: ['default', 'jest-junit'],
  coverageDirectory: '<rootDir>/../../coverage/PROJ_NAME',
  coverageReporters: ['text-summary', 'html'],
  setupFilesAfterEnv: ['<rootDir>/setup-jest.ts'],
  transformIgnorePatterns: ['node_modules/(?!(jest-test))'],
  testPathIgnorePatterns: ['<rootDir>/e2e/']
};

Jest unit test added: !141 (merged)

Source code files could benefit from more inline documentation. Javadoc-style.

Lots and lots of documentation added.

See #175 (closed) for merge requests.

See Gitlab pages: https://ghsc.code-pages.usgs.gov/nshmp/nshmp-apps/

Public api files remove !270 (merged) as they are not needed.

I have completed the technical code review as documented above. I've re-assigned this to @bclayton for reconciliation.

/cc @pmpowers

assigned to @bclayton and unassigned @emartinez

mentioned in merge request !109 (merged)

changed milestone to %nshmp-apps Review

mentioned in merge request !139 (merged)