Skip to content

only trigger onAny when no event registered for the event name, not interceptor#66

Merged
sanjomo merged 1 commit intomainfrom
sanjomo-patch-2
Dec 2, 2025
Merged

only trigger onAny when no event registered for the event name, not interceptor#66
sanjomo merged 1 commit intomainfrom
sanjomo-patch-2

Conversation

@sanjomo
Copy link
Member

@sanjomo sanjomo commented Nov 30, 2025

Description

Fixed the oversight during catchalleventlistener feature shipped

Type of Change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Documentation update
  • Code refactoring
  • Test improvements
  • Build/tooling changes

Related Issue

Closes #(issue number)

Changes Made

Testing

  • All existing tests pass
  • New tests added for new functionality
  • Tests pass locally with mvn test
  • Integration tests pass (if applicable)

Checklist

  • Code follows project coding standards
  • Self-review completed
  • Code is commented where necessary
  • Documentation updated (if needed)
  • Commit messages follow conventional format
  • No merge conflicts
  • All CI checks pass

Additional Notes

Any additional information, screenshots, or context that reviewers should know.

Summary by CodeRabbit

  • Bug Fixes
    • Event interceptors now only execute when event listeners are present for the event, improving efficiency by eliminating unnecessary interceptor invocations.

✏️ Tip: You can customize this high-level summary in your review settings.

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Nov 30, 2025

Walkthrough

The PR modifies event handling control flow in Namespace.java by relocating EventInterceptor invocation from a separate post-listener loop to execute conditionally within the DataListener processing block. Interceptors now only run when event listeners exist for the event.

Changes

Cohort / File(s) Summary
EventInterceptor Control Flow
netty-socketio-core/src/main/java/com/socketio4j/socketio/namespace/Namespace.java
Moved EventInterceptor handler invocation inside the DataListener processing block, making interceptor execution conditional on the presence of event listeners. No changes to exception handling or ack flow.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

  • Key areas requiring attention:
    • Verify the behavioral change (interceptors only running when listeners exist) doesn't violate expected interceptor semantics or break existing code that relies on interceptors executing regardless of listener presence
    • Confirm that conditional interceptor invocation aligns with the intended event processing contract
    • Review any dependent code or tests that assume interceptors always execute

Possibly related PRs

  • PR #60: Modifies Namespace.onEvent and EventInterceptor invocation placement; shares the same control flow and listener/interceptor logic interaction patterns

Suggested reviewers

  • NeatGuyCoding

Poem

🐰 The interceptor hops inside the gate,
Only when listeners await,
No more running solo through the night,
Control flow now feels just right!

Pre-merge checks and finishing touches

❌ Failed checks (3 warnings)
Check name Status Explanation Resolution
Description check ⚠️ Warning The description is largely incomplete with missing critical sections; 'Changes Made' is empty, 'Testing' shows no tests completed, and the description of the bug fix ('oversight during catchalleventlistener feature') is vague without explaining what was actually fixed. Fill in the 'Changes Made' section with specific details, check relevant testing boxes or explain why they cannot be completed, and clarify what oversight was fixed and why this change is necessary.
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
Title check ⚠️ Warning The PR title states 'only trigger onAny when no event registered for the event name, not interceptor' but the actual change moves EventInterceptor invocation inside the DataListener processing block, making interceptors conditional on listeners existing—the opposite of what the title suggests. Revise the title to accurately reflect the change, such as: 'Make EventInterceptor invocation conditional on DataListener presence' or 'Only invoke EventInterceptor when event listeners are registered'.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch sanjomo-patch-2

Comment @coderabbitai help to get the list of available commands and usage tips.

@sanjomo sanjomo changed the title only trigger onAny without registered event not interceptor only trigger onAny when no event registered for the event name, not interceptor Nov 30, 2025
@sanjomo sanjomo requested a review from NeatGuyCoding December 1, 2025 05:45
Copy link
Collaborator

@NeatGuyCoding NeatGuyCoding left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please update the related documents @coderabbitai

@sanjomo
Copy link
Member Author

sanjomo commented Dec 2, 2025

@NeatGuyCoding already updated doc stands true , this is a nit

@sanjomo sanjomo merged commit 9945958 into main Dec 2, 2025
8 checks passed
@sanjomo sanjomo deleted the sanjomo-patch-2 branch December 2, 2025 13:33
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.

2 participants