Sentry implementation

[mdellabitta]

Important

Integrates Sentry for error tracking, updates AWS SDK usage, refactors ThumbnailApi, and updates CI workflows and dependencies.

• Sentry Integration:
  • Adds Sentry for error tracking in src/instrumentation.js.
  • Initializes Sentry in src/app.ts and sets up error handling.
• AWS SDK Update:
  • Replaces aws-sdk with @aws-sdk/client-s3 and @aws-sdk/client-sqs in src/ThumbnailApi.ts.
  • Updates S3 and SQS client usage in src/app.ts and test/integration.ts.
• Code Refactoring:
  • Refactors ThumbnailApi class in src/ThumbnailApi.ts for improved error handling and async operations.
  • Updates getS3Url, lookupImageInS3, and queueToThumbnailCache methods.
• CI/CD Workflows:
  • Updates Node.js and ECR GitHub Actions workflows to use actions/checkout@v4 and actions/setup-node@v4.
  • Adds Sentry sourcemap upload step in ecr.yml.
• Dependencies and Configuration:
  • Updates package.json with new dependencies and scripts for Sentry and Prettier.
  • Adds .prettierrc.json and eslint.config.mjs for code formatting and linting.
  • Changes Node.js version to lts/Jod in .nvmrc and Dockerfile.

^{This description was created by for 707908f. It will automatically update as commits are pushed.}

[ellipsis-dev]

👍 Looks good to me! Reviewed everything up to 5443210 in 51 seconds

More details

• Looked at 1525 lines of code in 13 files
• Skipped 3 files when reviewing.
• Skipped posting 8 drafted comments based on config settings.

1. .github/workflows/ecr.yml:14

• Draft comment:
  Ensure that actions/setup-node@v4 is compatible with your project requirements and any other tools you are using. This update should be tested to confirm it doesn't introduce any issues.
• Reason this comment was not posted:
  Confidence changes required: 50%
  The change from actions/checkout@v3 to actions/checkout@v4 is correct as it updates to the latest version. However, the actions/setup-node@v4 should be checked for compatibility with the rest of the workflow.

2. .github/workflows/ecr.yml:29

• Draft comment:
  Good use of npm ci for clean installation of dependencies in CI environments.
• Reason this comment was not posted:
  Confidence changes required: 0%
  The npm ci command is used correctly here for installing dependencies in a clean state, which is suitable for CI environments.

3. .github/workflows/node.js.yml:22

• Draft comment:
  Good use of npm ci for clean installation of dependencies in CI environments.
• Reason this comment was not posted:
  Confidence changes required: 0%
  The npm ci command is used correctly here for installing dependencies in a clean state, which is suitable for CI environments.

4. .github/workflows/node.js.yml:21

• Draft comment:
  Using node-version-file to specify Node.js version from .nvmrc ensures consistency across environments.
• Reason this comment was not posted:
  Confidence changes required: 0%
  The node-version-file is correctly used to specify the Node.js version from .nvmrc. This ensures consistency across environments.

5. .github/workflows/ecr.yml:27

• Draft comment:
  Using node-version-file to specify Node.js version from .nvmrc ensures consistency across environments.
• Reason this comment was not posted:
  Confidence changes required: 0%
  The node-version-file is correctly used to specify the Node.js version from .nvmrc. This ensures consistency across environments.

6. .github/workflows/ecr.yml:45

• Draft comment:
  Ensure that globally installing tsc is necessary and doesn't conflict with any local TypeScript versions specified in your project.
• Reason this comment was not posted:
  Confidence changes required: 30%
  The npm install tsc -g command is used to globally install TypeScript compiler, which is necessary for the build process.

7. .github/workflows/node.js.yml:23

• Draft comment:
  Ensure that globally installing tsc is necessary and doesn't conflict with any local TypeScript versions specified in your project.
• Reason this comment was not posted:
  Confidence changes required: 30%
  The npm install tsc -g command is used to globally install TypeScript compiler, which is necessary for the build process.

8. Dockerfile:6

• Draft comment:
  Good use of npm ci for clean and consistent installation of dependencies in Dockerfile.
• Reason this comment was not posted:
  Confidence changes required: 0%
  The Dockerfile uses npm ci which is appropriate for CI/CD environments to ensure a clean and consistent install of dependencies.

Workflow ID: wflow_reFTQpIZRIZkzdJr

---

You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

[ellipsis-dev]

👍 Looks good to me! Incremental review on 4ab9ee5 in 16 seconds

More details

• Looked at 67 lines of code in 1 files
• Skipped 0 files when reviewing.
• Skipped posting 2 drafted comments based on config settings.

1. src/ThumbnailApi.ts:84

• Draft comment:
  Logging the entire error object may not be necessary. Consider logging just the error message for better readability.

console.error("Sending %s for %s: %s", code, itemId, error?.message);

• Reason this comment was not posted:
  Confidence changes required: 50%
  The error logging in sendError should include the error message, not the entire error object, for better readability.

2. src/ThumbnailApi.ts:334

• Draft comment:
  Consider moving the addHeader function outside of getHeadersFromTarget for better readability and performance.

function addHeader(result: Map<string, string>, headers: Headers, header: string) {
  if (headers.has(header)) {
    result.set(header, headers.get(header));
  }
}

• Reason this comment was not posted:
  Confidence changes required: 50%
  The getHeadersFromTarget function uses a helper function addHeader which is defined inside it. This is unnecessary and can be moved outside for better readability and performance.

Workflow ID: wflow_HshUkEi9z7IKVPpP

---

You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

[ellipsis-dev]

❌ Changes requested. Incremental review on d82313d in 56 seconds

More details

• Looked at 197 lines of code in 1 files
• Skipped 0 files when reviewing.
• Skipped posting 2 drafted comments based on config settings.

1. test/unit.ts:104

• Draft comment:
  The getCacheHeaders function is expected to return a Map, but the test is treating it as if it returns an object. This could lead to confusion or errors if the implementation changes.

  t.is(result.get("Cache-Control"), "public, max-age=2");

• Reason this comment was not posted:
  Comment looked like it was already resolved.

2. test/unit.ts:151

• Draft comment:
  The getHeadersFromTarget function is expected to return a Map, but the test is treating it as if it returns an object. This could lead to confusion or errors if the implementation changes.

  t.is(responseHeaders.get("Last-Modified"), headers.get("Last-Modified"));

• Reason this comment was not posted:
  Marked as duplicate.

Workflow ID: wflow_6jL8MOP7rDF13GSy

---

Want Ellipsis to fix these issues? Tag @ellipsis-dev in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

[ellipsis-dev]

The type annotation for the entry parameter in the forEach method is incorrect. The second element of the tuple should be string | undefined instead of never. This is because the testData object contains both strings and undefined as values.

Suggested change

	Object.entries(testData).forEach((entry: [string, never]): void => {
	Object.entries(testData).forEach((entry: [string, string | undefined]): void => {

[ellipsis-dev]

👍 Looks good to me! Incremental review on 47dab4e in 26 seconds

More details

• Looked at 21 lines of code in 1 files
• Skipped 0 files when reviewing.
• Skipped posting 1 drafted comments based on config settings.

1. test/unit.ts:23

• Draft comment:
  The type annotation for result should be string | undefined to match the possible return values of thumb.getItemId.
• Reason this comment was not posted:
  Comment was on unchanged code.

Workflow ID: wflow_JM78qBz9SE2HrCJs

---

You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

[ellipsis-dev]

👍 Looks good to me! Incremental review on 707908f in 55 seconds

More details

• Looked at 229 lines of code in 3 files
• Skipped 0 files when reviewing.
• Skipped posting 2 drafted comments based on config settings.

1. src/ThumbnailApi.ts:260

• Draft comment:
  Using Promise.reject for error handling is not ideal. Consider throwing an error instead for better stack trace and error propagation.

      throw new Error("Couldn't find image URL in record.");

• Reason this comment was not posted:
  Decided after close inspection that this draft comment was likely wrong and/or not actionable:
  The method is async and returns Promise. Both Promise.reject and throw would work here. Promise.reject is used consistently in other error cases in this file (lines 111, 116, 126, 266, 270). Changing just one instance would make the code less consistent. The stack trace argument is not compelling since async/await provides good stack traces either way.
  The comment makes a valid point that throw is generally preferred in async functions. The current error handling could be improved by using Error objects consistently.
  While throw might be marginally better, changing just one instance would reduce consistency. Any error handling improvements should be done systematically across the file.
  Delete the comment. While the suggestion has merit, it would reduce code consistency to change just this one instance, and the current approach works fine.

2. src/ThumbnailApi.ts:252

• Draft comment:
  Using Promise.reject for error handling is not ideal. Consider throwing an error instead for better stack trace and error propagation. This applies to all instances in this method.
• Reason this comment was not posted:
  Marked as duplicate.

Workflow ID: wflow_9wVu411kThyO8Ttv

---

You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.