Make an IRC listener connection follow its session's nick (closes #184)

When the session's nick was changed from another client, such as the
web client over the HTTP API, the IRC listener connection kept the old
nick and sent its messages, joins and later NICK changes under it. The
relay now gives the connection the new nick when it delivers a NICK
change whose old nick is the connection's own: nicks are unique, so its
own session made that change. As the relay goroutine can now change the
nick, the command handlers read it under the connection's lock, through
currentNick, as the relay already did.

Model: opus-5-5

internal/cli/app.go

@@ -96,7 +96,20 @@ func (a *App) handleInput(text string) {
 		a.ui.AddStatus(
 			"[red]Send error: " + err.Error(),
 		)
+
+		return
 	}
+
+	timestamp := time.Now().Format(timeFormat)
+
+	a.mu.Lock()
+	nick := a.nick
+	a.mu.Unlock()
+
+	a.ui.AddLine(target, fmt.Sprintf(
+		"[gray]%s [green]<%s>[white] %s",
+		timestamp, nick, text,
+	))
 }
 
 func (a *App) handleCommand(text string) {
@@ -288,6 +301,9 @@ func (a *App) cmdJoin(channel string) {
 	a.mu.Unlock()
 
 	a.ui.SwitchToBuffer(channel)
+	a.ui.AddLine(channel,
+		"[yellow]*** Joined "+channel,
+	)
 	a.ui.SetStatus(nick, channel, "connected")
 }
 
@@ -322,6 +338,10 @@ func (a *App) cmdPart(channel string) {
 		return
 	}
 
+	a.ui.AddLine(channel,
+		"[yellow]*** Left "+channel,
+	)
+
 	a.mu.Lock()
 	if a.target == channel {
 		a.target = ""
@@ -348,6 +368,7 @@ func (a *App) cmdMsg(args string) {
 
 	a.mu.Lock()
 	connected := a.connected
+	nick := a.nick
 	a.mu.Unlock()
 
 	if !connected {
@@ -365,7 +386,16 @@ func (a *App) cmdMsg(args string) {
 		a.ui.AddStatus(fmt.Sprintf(
 			"[red]Send failed: %v", err,
 		))
+
+		return
 	}
+
+	timestamp := time.Now().Format(timeFormat)
+
+	a.ui.AddLine(target, fmt.Sprintf(
+		"[gray]%s [green]<%s>[white] %s",
+		timestamp, nick, text,
+	))
 }
 
 func (a *App) cmdQuery(nick string) {
@@ -755,10 +785,12 @@ func (a *App) handlePrivmsgEvent(
 	lines := msg.BodyLines()
 	text := strings.Join(lines, " ")
 
-	// A direct message shows in the buffer of the other user:
-	// the recipient of one the user sent, else the sender.
+	if msg.From == myNick {
+		return
+	}
+
 	target := msg.To
-	if !strings.HasPrefix(target, "#") && msg.From != myNick {
+	if !strings.HasPrefix(target, "#") {
 		target = msg.From
 	}
 
@@ -781,35 +813,31 @@ func (a *App) handleJoinEvent(
 	))
 }
 
-// handlePartEvent shows a part in the channel's buffer. The
-// user's own part does not mark the channel unread: the user
-// has just left it.
+// handlePartEvent shows another user's part. The server
+// also sends the user's own part back to it, which cmdPart
+// has already shown; showing it again would add a second
+// line and mark the channel just left as unread.
 func (a *App) handlePartEvent(
 	msg *api.Message, timestamp, myNick string,
 ) {
-	if msg.To == "" {
+	if msg.To == "" || msg.From == myNick {
 		return
 	}
 
 	lines := msg.BodyLines()
 	reason := strings.Join(lines, " ")
 
-	line := fmt.Sprintf(
-		"[gray]%s [yellow]*** %s has left %s",
-		timestamp, msg.From, msg.To,
-	)
-
 	if reason != "" {
-		line += " (" + reason + ")"
+		a.ui.AddLine(msg.To, fmt.Sprintf(
+			"[gray]%s [yellow]*** %s has left %s (%s)",
+			timestamp, msg.From, msg.To, reason,
+		))
+	} else {
+		a.ui.AddLine(msg.To, fmt.Sprintf(
+			"[gray]%s [yellow]*** %s has left %s",
+			timestamp, msg.From, msg.To,
+		))
 	}
-
-	if msg.From == myNick {
-		a.ui.AddLineAsRead(msg.To, line)
-
-		return
-	}
-
-	a.ui.AddLine(msg.To, line)
 }
 
 func (a *App) handleQuitEvent(

internal/cli/app_internal_test.go

@@ -1,8 +1,6 @@
 package cli
 
 import (
-	"net/http"
-	"net/http/httptest"
 	"testing"
 
 	"github.com/gdamore/tcell/v2"
@@ -10,15 +8,6 @@ import (
 	"sneak.berlin/go/neoirc/pkg/irc"
 )
 
-// The user's nick, a channel it is in, another user's nick and
-// a message text, for the tests below.
-const (
-	testNick      = "erin"
-	testChannel   = "#chan"
-	testOtherNick = "bob"
-	testText      = "hello"
-)
-
 func TestMemberPrefix(t *testing.T) {
 	t.Parallel()
 
@@ -64,190 +53,48 @@ func TestMemberPrefix(t *testing.T) {
 }
 
 // TestPartEvent checks that a PART event adds a line to the
-// channel's buffer and marks it unread, except that the
-// user's own part does not mark it unread.
+// channel's buffer and marks it unread, except for the
+// user's own part, which cmdPart has already shown.
 func TestPartEvent(t *testing.T) {
 	t.Parallel()
 
 	tests := []struct {
 		name       string
 		from       string
+		wantLines  int
 		wantUnread int
 	}{
-		{name: "own part", from: testNick, wantUnread: 0},
-		{name: "other member", from: testOtherNick, wantUnread: 1},
+		{name: "own part", from: "erin", wantLines: 0, wantUnread: 0},
+		{name: "other member", from: "bob", wantLines: 1, wantUnread: 1},
 	}
 
 	for _, test := range tests {
 		t.Run(test.name, func(t *testing.T) {
 			t.Parallel()
 
-			app := &App{ui: NewUI(), nick: testNick}
+			app := &App{ui: NewUI(), nick: "erin"}
 			runUI(t, app.ui)
 
 			app.handleServerMessage(&api.Message{
-				Command: irc.CmdPart, From: test.from, To: testChannel,
+				Command: irc.CmdPart, From: test.from, To: "#chan",
 			})
 
-			lines, unread := bufferState(app.ui, testChannel)
-			if lines != 1 || unread != test.wantUnread {
-				t.Fatalf("got %d lines and %d unread, want 1 and %d",
-					lines, unread, test.wantUnread)
+			var lines, unread int
+
+			app.ui.app.QueueUpdate(func() {
+				buf := app.ui.getOrCreateBuffer("#chan")
+				lines = len(buf.Lines)
+				unread = buf.Unread
+			})
+
+			if lines != test.wantLines || unread != test.wantUnread {
+				t.Fatalf("got %d lines and %d unread, want %d and %d",
+					lines, unread, test.wantLines, test.wantUnread)
 			}
 		})
 	}
 }
 
-// TestOwnCommandShownOnce checks that a message, join or
-// part made from neoirc-cli shows as one line in the
-// channel's buffer once the server's event for it arrives,
-// and does not mark the channel unread.
-func TestOwnCommandShownOnce(t *testing.T) {
-	t.Parallel()
-
-	tests := []struct {
-		name  string
-		send  func(app *App)
-		event api.Message
-	}{
-		{
-			name: "message",
-			send: func(app *App) { app.handleInput(testText) },
-			event: api.Message{
-				Command: irc.CmdPrivmsg, From: testNick, To: testChannel,
-				Body: []string{testText},
-			},
-		},
-		{
-			name: "join",
-			send: func(app *App) { app.cmdJoin(testChannel) },
-			event: api.Message{
-				Command: irc.CmdJoin, From: testNick, To: testChannel,
-			},
-		},
-		{
-			name: "part",
-			send: func(app *App) { app.cmdPart(testChannel) },
-			event: api.Message{
-				Command: irc.CmdPart, From: testNick, To: testChannel,
-			},
-		},
-	}
-
-	for _, test := range tests {
-		t.Run(test.name, func(t *testing.T) {
-			t.Parallel()
-
-			// The server accepts every command and answers nothing.
-			server := httptest.NewServer(http.HandlerFunc(
-				func(http.ResponseWriter, *http.Request) {},
-			))
-			t.Cleanup(server.Close)
-
-			app := &App{
-				ui:        NewUI(),
-				client:    api.NewClient(server.URL),
-				nick:      testNick,
-				target:    testChannel,
-				connected: true,
-			}
-			runUI(t, app.ui)
-			app.ui.SwitchToBuffer(testChannel)
-
-			test.send(app)
-			app.handleServerMessage(&test.event)
-
-			lines, unread := bufferState(app.ui, testChannel)
-			if lines != 1 || unread != 0 {
-				t.Fatalf("got %d lines and %d unread, want 1 and 0",
-					lines, unread)
-			}
-		})
-	}
-}
-
-// TestOwnEventFromOtherClient checks that a message, notice,
-// join or part that the user's session makes from another
-// client shows as one line in the buffer it belongs to.
-func TestOwnEventFromOtherClient(t *testing.T) {
-	t.Parallel()
-
-	tests := []struct {
-		name   string
-		event  api.Message
-		buffer string
-	}{
-		{
-			name: "channel message",
-			event: api.Message{
-				Command: irc.CmdPrivmsg, From: testNick, To: testChannel,
-				Body: []string{testText},
-			},
-			buffer: testChannel,
-		},
-		{
-			name: "direct message",
-			event: api.Message{
-				Command: irc.CmdPrivmsg, From: testNick, To: testOtherNick,
-				Body: []string{testText},
-			},
-			buffer: testOtherNick,
-		},
-		{
-			name: "notice",
-			event: api.Message{
-				Command: irc.CmdNotice, From: testNick, To: testChannel,
-				Body: []string{testText},
-			},
-			buffer: "(status)",
-		},
-		{
-			name: "join",
-			event: api.Message{
-				Command: irc.CmdJoin, From: testNick, To: testChannel,
-			},
-			buffer: testChannel,
-		},
-		{
-			name: "part",
-			event: api.Message{
-				Command: irc.CmdPart, From: testNick, To: testChannel,
-			},
-			buffer: testChannel,
-		},
-	}
-
-	for _, test := range tests {
-		t.Run(test.name, func(t *testing.T) {
-			t.Parallel()
-
-			app := &App{ui: NewUI(), nick: testNick}
-			runUI(t, app.ui)
-
-			app.handleServerMessage(&test.event)
-
-			lines, _ := bufferState(app.ui, test.buffer)
-			if lines != 1 {
-				t.Fatalf("got %d lines in %s, want 1", lines, test.buffer)
-			}
-		})
-	}
-}
-
-// bufferState returns the number of lines in the named
-// buffer and how many of them are unread.
-func bufferState(ui *UI, name string) (int, int) {
-	var lines, unread int
-
-	ui.app.QueueUpdate(func() {
-		buf := ui.getOrCreateBuffer(name)
-		lines = len(buf.Lines)
-		unread = buf.Unread
-	})
-
-	return lines, unread
-}
-
 // runUI runs ui on a simulated screen until the test ends.
 func runUI(t *testing.T, ui *UI) {
 	t.Helper()

internal/cli/ui.go

@@ -95,19 +95,6 @@ func (ui *UI) AddLine(bufferName, line string) {
 	})
 }
 
-// AddLineAsRead adds a line to the specified buffer, as
-// AddLine does, but does not mark the buffer unread.
-func (ui *UI) AddLineAsRead(bufferName, line string) {
-	ui.app.QueueUpdateDraw(func() {
-		buf := ui.getOrCreateBuffer(bufferName)
-		buf.Lines = append(buf.Lines, line)
-
-		if ui.buffers[ui.currentBuffer] == buf {
-			_, _ = fmt.Fprintln(ui.messages, line)
-		}
-	})
-}
-
 // AddStatus adds a line to the status buffer.
 func (ui *UI) AddStatus(line string) {
 	ts := time.Now().Format("15:04")

internal/db/queries.go

@@ -264,25 +264,6 @@ func (database *Database) GetSessionByNick(
 	return sessionID, nil
 }
 
-// GetSessionNick returns the nick a session holds.
-func (database *Database) GetSessionNick(
-	ctx context.Context,
-	sessionID int64,
-) (string, error) {
-	var nick string
-
-	err := database.conn.QueryRowContext(
-		ctx,
-		"SELECT nick FROM sessions WHERE id = ?",
-		sessionID,
-	).Scan(&nick)
-	if err != nil {
-		return "", fmt.Errorf("get session nick: %w", err)
-	}
-
-	return nick, nil
-}
-
 // SessionHostInfo holds the username, hostname, and IP
 // for a session.
 type SessionHostInfo struct {

internal/ircserver/commands.go

@@ -92,10 +92,9 @@ func (c *Conn) handleNick(
 		return
 	}
 
-	// Take the nick the session holds, not newNick: another
-	// client of the session may have changed it again since
-	// ChangeNick stored newNick.
-	c.loadSessionNick(ctx)
+	c.mu.Lock()
+	c.nick = newNick
+	c.mu.Unlock()
 
 	// Echo NICK change to the client on wire.
 	c.send(FormatMessage(oldMask, "NICK", newNick))

internal/ircserver/conn.go

@@ -313,22 +313,6 @@ func (c *Conn) currentNick() string {
 	return c.nick
 }
 
-// loadSessionNick sets c.nick to the nick the connection's
-// session holds in the database, read by session ID. The
-// connection's own NICK and the relay passing on a NICK line
-// both call it, and it reads and sets the nick under c.mu, so
-// whichever runs last leaves the connection on its session's
-// nick. If the read fails, c.nick is left as it is.
-func (c *Conn) loadSessionNick(ctx context.Context) {
-	c.mu.Lock()
-	defer c.mu.Unlock()
-
-	nick, err := c.database.GetSessionNick(ctx, c.sessionID)
-	if err == nil {
-		c.nick = nick
-	}
-}
-
 // send writes a formatted IRC line to the connection.
 func (c *Conn) send(line string) {
 	c.sendWithin(writeTimeout, line)

internal/ircserver/relay.go

@@ -92,7 +92,7 @@ func (c *Conn) drainQueue(ctx context.Context) bool {
 //
 //nolint:cyclop // dispatch table
 func (c *Conn) deliverIRCMessage(
-	ctx context.Context,
+	_ context.Context,
 	msg *db.IRCMessage,
 ) {
 	command := msg.Command
@@ -120,7 +120,7 @@ func (c *Conn) deliverIRCMessage(
 	case command == irc.CmdPart:
 		c.deliverPart(msg, text)
 	case command == irc.CmdNick:
-		c.deliverNickChange(ctx, msg, text)
+		c.deliverNickChange(msg, text)
 	case command == irc.CmdQuit:
 		c.deliverQuitMsg(msg, text)
 	case command == irc.CmdTopic:
@@ -230,17 +230,21 @@ func (c *Conn) deliverPart(msg *db.IRCMessage, text string) {
 	}
 }
 
-// deliverNickChange sends a NICK change notification. The
-// change may be its own session's, made from another client,
-// so the connection first takes the nick its session holds
-// now. It does not go by the line's old nick: the connection
-// may have taken that nick after another user gave it up.
+// deliverNickChange sends a NICK change notification. When
+// the old nick is the connection's own, the change was made
+// from another client of its session, as nicks are unique,
+// and the connection takes the new nick too.
 func (c *Conn) deliverNickChange(
-	ctx context.Context,
 	msg *db.IRCMessage,
 	newNick string,
 ) {
-	c.loadSessionNick(ctx)
+	c.mu.Lock()
+
+	if msg.From == c.nick {
+		c.nick = newNick
+	}
+
+	c.mu.Unlock()
 
 	prefix := msg.From + "!" + msg.From + "@*"
 

internal/ircserver/relay_test.go

@@ -655,43 +655,3 @@ func TestRelayNickChangeFromHTTPAPI(t *testing.T) {
 		}
 	}
 }
-
-// TestRelayNickChangeKeepsTakenNick checks that an IRC
-// listener connection which takes a nick another user has
-// given up keeps it when that user's change reaches it
-// afterwards: what it then sends goes out under the nick its
-// session holds.
-func TestRelayNickChangeKeepsTakenNick(t *testing.T) {
-	t.Parallel()
-
-	env := newTestEnv(t)
-
-	bob := env.dial(t)
-	bob.register("bob")
-	bob.sendAndExpect("NICK bob2", " NICK bob2")
-
-	alice := env.dial(t)
-	alice.register("alice")
-	alice.sendAndExpect("NICK bob", " NICK bob")
-	alice.joinAndDrain("#chan")
-
-	bob.joinAndDrain("#chan")
-
-	// Bob's change reaches alice only now, as it does when she
-	// takes his old nick before her connection has passed the
-	// change on. She holds bob now, so it is queued to bob.
-	env.queueMessage(t, "bob", "NICK", "bob", "", []string{"bob2"})
-
-	alice.readUntil(func(l string) bool {
-		return l == ":bob!bob@* NICK bob2"
-	})
-
-	alice.send("PRIVMSG #chan :hi there")
-
-	const want = ":bob!bob@* PRIVMSG #chan :hi there"
-
-	got := bob.readLine()
-	if got != want {
-		t.Errorf("got %q, want %q", got, want)
-	}
-}