Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix chanlist in nick change events #117

Merged
merged 5 commits into from
Jan 8, 2025

Conversation

jkhsjdhjs
Copy link

This fixes nick changes not being bridged to matrix, due to the chanlist being empty in nick change events.

Fix matrix-org/matrix-appservice-irc#1776

@jkhsjdhjs jkhsjdhjs requested a review from a team as a code owner July 27, 2024 00:37
We know that nickChannel.users.get() always returns a value, because we
check it via .has() beforehand.
Signed-off-by: jkhsjdhjs <[email protected]>
@jkhsjdhjs jkhsjdhjs force-pushed the fix/nick_event_chanlist branch from a439c8b to 7fefffe Compare August 1, 2024 21:30
@jkhsjdhjs jkhsjdhjs mentioned this pull request Sep 13, 2024
@jkhsjdhjs
Copy link
Author

@tadzik friendly ping

@tadzik
Copy link

tadzik commented Nov 12, 2024

Having tested it against the IRC bridge, it doesn't seem to solve the issue – both with and without the patch the event fires in https://github.com/matrix-org/matrix-appservice-irc/blob/develop/src/irc/BridgedClient.ts#L294. It then never does anything because this.nick is set to the connection instance's nick (could be MatrixBot or one of the matrix users' ghosts).

Did it fix anything in your case? Is there any situation when this should fire but doesn't? Could you share steps to reproduce this?

@jkhsjdhjs
Copy link
Author

That's a different event I think, can't tell exactly because I no longer have access to my dev instance, but I think this is the event that isn't triggered currently: https://github.com/matrix-org/matrix-appservice-irc/blob/7edfec7bd45cf3695904be7da3633d2e4475f90c/src/irc/IrcEventBroker.ts#L399

Or to be precise: The event handler is triggered, the nick event is emitted, but chans is empty and thus the matrix puppets aren't updated.

To reproduce you can just have an irc user (not a puppet) change their nick. The corresponding matrix puppet will not be updated.

#103 attempted to fix this previously and initially had it correct, but then committed this to fix the typecheckers complaints: 955f6d4

This commit, however, doesn't make any sense and actually breaks the state tracking, as it saves booleans instead of channel lists in the nickChannel map. This PR reverts the commit and fixes the typecheckers complaints this way: 0f1caa2

@@ -630,9 +630,8 @@ export class Client extends (EventEmitter as unknown as new () => TypedEmitter<C

// finding what channels a user is in
this.state.chans.forEach((nickChannel, channame) => {
const chanUser = message.nick && nickChannel.users.get(message.nick);
if (message.nick && chanUser) {
Copy link

Choose a reason for hiding this comment

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

This if-statement checks if chanUser is truthy, and an empty string is not.
So nick change events don't get past this check, if the user has no channel modes set (normal user).
The event still fires in matrix-appservice-irc, but with a channel list that's always empty, so nothing happens.

@f0x52
Copy link

f0x52 commented Dec 1, 2024

I was able to reproduce nick changes not getting bridged properly, and this PR correctly fixes the issue by checking if the old nick was in the userlist for the channel, instead of relying on the stored usermode string being truthy (an empty string isn't).

f0x52 added a commit to f0x52/node-irc that referenced this pull request Dec 2, 2024
manual cherry-pick fix from PR matrix-org#117 by jkhsjdhjs on upstream github
@jkhsjdhjs
Copy link
Author

@tadzik

@tadzik
Copy link

tadzik commented Jan 8, 2025

Okay; gave it a fresh try and experimented a bit, since it was clearly something wrong with me here :)

Turns out that for this to work, membershipLists.global.ircToMatrix.incremental needs to be enabled. In that case the patch indeed fixes the nick change problem.

@tadzik tadzik merged commit 1a4e183 into matrix-org:master Jan 8, 2025
5 checks passed
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.

Nick changes on IRC are not recognized
3 participants