Skip to content

Conversation

@kushalshit27
Copy link
Contributor

@kushalshit27 kushalshit27 commented Oct 8, 2025

🔧 Changes

Fix handler execution order to ensure clientGrants processes before logStreams

Solution:

  • Added @order('70') decorator to logStreams to ensure it executes after clientGrants (priority 60)

📚 References

🔬 Testing

  • Unit Tests: ✅

📝 Checklist

  • All new/changed/fixed functionality is covered by tests (or N/A)
  • I have added documentation for all new/changed functionality (or N/A)

- src/tools/auth0/handlers/logStreams.ts: import 'order' decorator and apply it to processChanges method
- src/tools/auth0/index.ts: improve sortByOrder function to handle NaN cases and ensure proper sorting of handlers by order
@kushalshit27
Copy link
Contributor Author

@claude

@github-actions

This comment was marked as outdated.

@codecov-commenter
Copy link

codecov-commenter commented Oct 8, 2025

Codecov Report

❌ Patch coverage is 42.85714% with 4 lines in your changes missing coverage. Please review.
✅ Project coverage is 80.17%. Comparing base (d3231b5) to head (8247248).

Files with missing lines Patch % Lines
src/tools/auth0/handlers/default.ts 50.00% 1 Missing and 1 partial ⚠️
src/tools/auth0/index.ts 0.00% 0 Missing and 2 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1175      +/-   ##
==========================================
- Coverage   80.23%   80.17%   -0.07%     
==========================================
  Files         134      134              
  Lines        5096     5100       +4     
  Branches     1013     1014       +1     
==========================================
  Hits         4089     4089              
- Misses        578      579       +1     
- Partials      429      432       +3     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@kushalshit27
Copy link
Contributor Author

@claude

@github-actions

This comment was marked as resolved.

@kushalshit27 kushalshit27 marked this pull request as ready for review October 8, 2025 17:58
@kushalshit27 kushalshit27 requested a review from a team as a code owner October 8, 2025 17:58
@kushalshit27 kushalshit27 changed the title fix: enhance log stream handling order fix: execution order to ensure clientGrants processes before logStreams Oct 8, 2025
@kushalshit27 kushalshit27 changed the title fix: execution order to ensure clientGrants processes before logStreams fix: execution order for logStreams Oct 8, 2025
@kushalshit27
Copy link
Contributor Author

@claude

@github-actions
Copy link

github-actions bot commented Oct 10, 2025

Claude finished @kushalshit27's task —— View job


Review Progress

  • Read project documentation
  • Review diff and changed files
  • Analyze code quality
  • Analyze security concerns
  • Analyze performance implications
  • Provide inline feedback
  • Provide overall assessment

Overall Assessment

❌ The PR has several issues that should be addressed before merging. While the order decorator validation is solid, the execution order choice creates ambiguity, the sorting function has a minor inefficiency, lacks defensive NaN handling, and there's no test coverage for the critical sorting logic.

Key Concerns:

  1. Order Priority Conflict: Using @order('70') for logStreams creates ambiguity with 4 other handlers at the same priority level. Consider using @order('65') for explicit positioning between clientGrants (60) and other handlers.
  2. Missing Test Coverage: The refactored sortByOrder function has no tests despite being critical for handler execution order.
  3. Minor Code Quality Issues: Unnecessary array copy and potential NaN edge case in sorting logic.

Strengths:

  • Order decorator validation is well-implemented and prevents invalid values
  • Documentation in sortByOrder is clear and helpful
  • The overall approach to fixing the execution order issue is sound

Job Run | Branch: DXCDT-1266-set-order-of-execution-for-tenant

@kushalshit27 kushalshit27 merged commit f0fc1b9 into master Oct 10, 2025
5 of 7 checks passed
@kushalshit27 kushalshit27 deleted the DXCDT-1266-set-order-of-execution-for-tenant branch October 10, 2025 06:58
@kushalshit27 kushalshit27 mentioned this pull request Oct 13, 2025
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.

Set order of execution for tenant

4 participants