Skip to content

Conversation

@vdua
Copy link
Collaborator

@vdua vdua commented Oct 26, 2025

  • error.stack is not a standard api

Please ensure your pull request adheres to the following guidelines:

  • make sure to link the related issues in this description
  • when merging / squashing, make sure the fixed issue references are visible in the commits, for easy compilation of release notes

Related Issues

Thanks for contributing!

Test URL

https://fix-error-stack--helix-rum-js--adobe.aem.live/test/static.html

- error.stack is not a standard api
@aem-code-sync
Copy link

aem-code-sync bot commented Oct 26, 2025

Hello, I'm the AEM Code Sync Bot and I will run some actions to deploy your branch and validate page speed.
In case there are problems, just click a checkbox below to rerun the respective action.

  • Re-run PSI checks
  • Re-sync branch
Commits

@adobe-bot
Copy link
Collaborator

adobe-bot commented Oct 26, 2025

Claude finished @vdua's task —— View job


Code Review Complete ✅

I've reviewed this PR for code quality, potential bugs, performance considerations, security concerns, and test coverage.

Todo List:

  • Read repository guidelines (CLAUDE.md)
  • Analyze the code changes in src/index.js
  • Review for code quality and best practices
  • Check for potential bugs or issues
  • Assess performance considerations
  • Evaluate security concerns
  • Review test coverage needs
  • Provide comprehensive feedback

Review Feedback

Positive Changes

Core Fix is Solid: The null check for error.stack at src/index.js:35 is a good defensive programming practice. Since error.stack is not standardized across all JavaScript environments, this prevents runtime errors when the property is undefined.

Proper Error Handling: The fix maintains the existing try-catch structure and gracefully handles cases where error.stack is not available.

🚨 Critical Issues

1. Unintended Content Addition (HIGH SEVERITY)
The PR accidentally adds 24 lines of unrelated documentation content (lines 113-134) that appears to be about "Coding Agent" functionality. This content:

  • Has no relation to the error.stack fix
  • Contains incomplete text that cuts off mid-sentence
  • Should be completely removed from this PR

2. Logic Issue in Stack Processing (MEDIUM SEVERITY)
At src/index.js:36-40, the code calls .shift() on the filtered array, but .shift() returns undefined if the array is empty. This could lead to:

errData.source = error.stack.split('\n')
    .filter((line) => line.match(/https?:\/\//)).shift() // Could be undefined
    .replace(/at ([^ ]+) \((.+)\)/, '$1@$2') // TypeError if undefined

Recommendation: Add a null check after .shift():

if (error.stack) {
  const stackLine = error.stack.split('\n')
      .filter((line) => line.match(/https?:\/\//)).shift();
  if (stackLine) {
    errData.source = stackLine
        .replace(/at ([^ ]+) \((.+)\)/, '$1@$2')
        .replace(/ at /, '@')
        .trim();
  }
}

📋 Code Quality & Best Practices

Indentation Inconsistency: The added if (error.stack) block uses 2 extra spaces of indentation compared to the original code style. Consider aligning with the existing 2-space indentation pattern.

🔒 Security Assessment

No Security Concerns: The changes don't introduce any security vulnerabilities. The error handling is defensive and doesn't expose sensitive information.

Performance Considerations

Minimal Performance Impact: The added null check is a simple boolean operation with negligible performance overhead. The fix actually prevents potential performance issues from runtime errors.

🧪 Test Coverage Assessment

Current Test Coverage: The existing test suite in /test/errors/ appears comprehensive with tests for various error scenarios.

Missing Test Case: Consider adding a test case for errors without a stack property to verify the new null check works correctly:

it('rum capture error without stack property', async () => {
  await test(() => {
    const errorWithoutStack = new Error('No stack');
    delete errorWithoutStack.stack; // Remove stack property
    throw errorWithoutStack;
  }, (source) => {
    assert.strictEqual(source, 'undefined error'); // Should fallback to default
  }, (target) => {
    assert.strictEqual(target, 'Error: No stack');
  }, config.queue);
});

🔧 Recommended Actions

  1. MUST FIX: Remove lines 113-134 (unrelated documentation content)
  2. SHOULD FIX: Add null check after .shift() to prevent potential TypeError
  3. CONSIDER: Add test case for error objects without stack property
  4. MINOR: Adjust indentation to match existing code style

@aem-code-sync
Copy link

aem-code-sync bot commented Oct 26, 2025

Page Scores Audits Google
📱 /test/static.html PERFORMANCE A11Y SEO BEST PRACTICES SI FCP LCP TBT CLS PSI
🖥️ /test/static.html PERFORMANCE A11Y SEO BEST PRACTICES SI FCP LCP TBT CLS PSI

@vdua vdua closed this Oct 26, 2025
@vdua vdua deleted the fix/error-stack branch October 26, 2025 16:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants