fix: address PR #121 review comments post-merge

migrations/002_conversations_discord_message_id.cjs

@@ -0,0 +1,33 @@
+/**
+ * Migration 002: Add discord_message_id to conversations table
+ *
+ * Stores the native Discord message ID alongside each conversation row so the
+ * dashboard can construct clickable jump URLs for individual messages.
+ * Existing rows will have NULL for this column (history before this migration).
+ */
+
+/** @param {import('node-pg-migrate').MigrationBuilder} pgm */
+exports.up = (pgm) => {
+  pgm.sql(`
+    ALTER TABLE conversations
+    ADD COLUMN IF NOT EXISTS discord_message_id TEXT
+  `);
+
+  pgm.sql(`
+    CREATE INDEX IF NOT EXISTS idx_conversations_discord_message_id 
+    ON conversations(discord_message_id) 
+    WHERE discord_message_id IS NOT NULL
+  `);
+};
+
+/** @param {import('node-pg-migrate').MigrationBuilder} pgm */
+exports.down = (pgm) => {
+  pgm.sql(`
+    DROP INDEX IF EXISTS idx_conversations_discord_message_id
+  `);
+
+  pgm.sql(`
+    ALTER TABLE conversations
+    DROP COLUMN IF EXISTS discord_message_id
+  `);
+};

src/api/routes/conversations.js

@@ -9,7 +9,7 @@ import { Router } from 'express';
 import { info, error as logError } from '../../logger.js';
 import { escapeIlike } from '../../utils/escapeIlike.js';
 import { rateLimit } from '../middleware/rateLimit.js';
-import { requireGuildAdmin, validateGuild } from './guilds.js';
+import { parsePagination, requireGuildAdmin, validateGuild } from './guilds.js';
 
 const router = Router({ mergeParams: true });
 
@@ -19,22 +19,6 @@ const conversationsRateLimit = rateLimit({ windowMs: 60 * 1000, max: 60 });
 /** Conversation grouping gap in minutes */
 const CONVERSATION_GAP_MINUTES = 15;
 
-/**
- * Parse pagination query params with defaults and capping.
- *
- * @param {Object} query - Express req.query
- * @returns {{ page: number, limit: number, offset: number }}
- */
-function parsePagination(query) {
-  let page = Number.parseInt(query.page, 10) || 1;
-  let limit = Number.parseInt(query.limit, 10) || 25;
-  if (page < 1) page = 1;
-  if (limit < 1) limit = 1;
-  if (limit > 100) limit = 100;
-  const offset = (page - 1) * limit;
-  return { page, limit, offset };
-}
-
 /**
  * Estimate token count from text content.
  * Rough heuristic: ~4 characters per token.
@@ -127,7 +111,7 @@ function buildConversationSummary(convo, guild) {
     ? firstMsg.content.slice(0, 100) + (firstMsg.content.length > 100 ? '…' : '')
     : '';
 
-  const channelName = guild?.channels?.cache?.get(convo.channelId)?.name || convo.channelId;
+  const channelName = guild?.channels?.cache?.get(convo.channelId)?.name || null;
 
   return {
     id: convo.id,
@@ -288,15 +272,19 @@ router.get('/', conversationsRateLimit, requireGuildAdmin, validateGuild, async
       values.push(req.query.channel);
     }
 
+    let fromFilterApplied = false;
     if (req.query.from && typeof req.query.from === 'string') {
       const from = new Date(req.query.from);
       if (!Number.isNaN(from.getTime())) {
         paramIndex++;
         whereParts.push(`created_at >= $${paramIndex}`);
         values.push(from.toISOString());
+        fromFilterApplied = true;
       }
-    } else {
+    }
+    if (!fromFilterApplied) {
       // Default: last 30 days to prevent unbounded scans on active servers
+      // Also applies when 'from' is provided but invalid, preventing unbounded queries
       paramIndex++;
       whereParts.push(`created_at >= $${paramIndex}`);
       values.push(new Date(Date.now() - 30 * 24 * 60 * 60 * 1000).toISOString());
@@ -313,17 +301,20 @@ router.get('/', conversationsRateLimit, requireGuildAdmin, validateGuild, async
 
     const whereClause = whereParts.join(' AND ');
 
-    // Fetch matching messages for grouping (capped at 5000 rows to prevent memory exhaustion)
+    // Fetch matching messages for grouping (capped at 10000 rows to prevent memory exhaustion)
     // Time-based grouping requires sorted rows; paginate after grouping
     const result = await dbPool.query(
       `SELECT id, channel_id, role, content, username, created_at
          FROM conversations
          WHERE ${whereClause}
-         ORDER BY created_at ASC
+         ORDER BY created_at DESC
          LIMIT 10000 -- capped to prevent runaway memory; 30-day default window keeps this reasonable`,
       values,
     );
 
+    // Reverse to ASC order so groupMessagesIntoConversations sees chronological messages.
+    // Fetching DESC first ensures we get the most recent 10k rows, not the oldest.
+    result.rows.reverse();
     const allConversations = groupMessagesIntoConversations(result.rows);
     const total = allConversations.length;
 
@@ -716,8 +707,20 @@ router.get('/flags', conversationsRateLimit, requireGuildAdmin, validateGuild, a
  *                         type: string
  *                         nullable: true
  *                         enum: [open, resolved, dismissed]
+ *                       discordMessageId:
+ *                         type: string
+ *                         nullable: true
+ *                         description: Native Discord message ID for constructing jump URLs
+ *                       messageUrl:
+ *                         type: string
+ *                         nullable: true
+ *                         description: Full Discord jump URL for the message (null if no discord_message_id)
  *                 channelId:
  *                   type: string
+ *                 channelName:
+ *                   type: string
+ *                   nullable: true
+ *                   description: Human-readable channel name from the Discord guild cache
  *                 duration:
  *                   type: integer
  *                   description: Duration in seconds
@@ -778,7 +781,7 @@ router.get(
       // Fetch messages in a bounded time window around the anchor (±2 hours)
       // to avoid loading the entire channel history
       const messagesResult = await dbPool.query(
-        `SELECT id, channel_id, role, content, username, created_at
+        `SELECT id, channel_id, role, content, username, created_at, discord_message_id
          FROM conversations
          WHERE guild_id = $1 AND channel_id = $2
            AND created_at BETWEEN ($3::timestamptz - interval '2 hours')
@@ -801,6 +804,7 @@ router.get(
         content: msg.content,
         username: msg.username,
         createdAt: msg.created_at,
+        discordMessageId: msg.discord_message_id || null,
       }));
 
       const durationMs = targetConvo.lastTime - targetConvo.firstTime;
@@ -824,14 +828,21 @@ router.get(
         }
       }
 
+      const channelName = req.guild?.channels?.cache?.get(anchor.channel_id)?.name || null;
+
       const enrichedMessages = messages.map((m) => ({
         ...m,
         flagStatus: flaggedMessageIds.get(m.id) || null,
+        messageUrl:
+          m.discordMessageId && guildId
+            ? `https://discord.com/channels/${guildId}/${anchor.channel_id}/${m.discordMessageId}`
+            : null,
       }));
 
       res.json({
         messages: enrichedMessages,
         channelId: anchor.channel_id,
+        channelName,
         duration: Math.round(durationMs / 1000),
         tokenEstimate: estimateTokens(messages.map((m) => m.content || '').join('')),
       });

src/api/routes/guilds.js

@@ -32,15 +32,12 @@ const MANAGE_GUILD_FLAG = 0x20;
 const MAX_CONTENT_LENGTH = 10000;
 
 /**
- * Parse pagination query params with defaults and capping.
+ * Parse pagination query parameters and return normalized page, limit, and offset.
  *
- * Currently used only by the moderation endpoint; the members endpoint
- * uses cursor-based pagination instead.
- *
- * @param {Object} query - Express req.query
- * @returns {{ page: number, limit: number, offset: number }}
+ * @param {Object} query - Query object (for example, Express `req.query`) possibly containing `page` and `limit`.
+ * @returns {{page: number, limit: number, offset: number}} page is at least 1, limit is between 1 and 100, offset equals `(page - 1) * limit`.
  */
-function parsePagination(query) {
+export function parsePagination(query) {
   let page = Number.parseInt(query.page, 10) || 1;
   let limit = Number.parseInt(query.limit, 10) || 25;
   if (page < 1) page = 1;

src/modules/ai.js

@@ -195,14 +195,16 @@ export async function getHistoryAsync(channelId) {
 }
 
 /**
- * Add message to conversation history
- * Writes to both in-memory cache and DB (write-through)
- * @param {string} channelId - Channel ID
- * @param {string} role - Message role (user/assistant)
- * @param {string} content - Message content
- * @param {string} [username] - Optional username
+ * Append a message to the in-memory conversation history for a channel and attempt to persist it to the database.
+ *
+ * Also attempts a fire-and-forget write to the DB; database errors are logged and do not throw.
+ * @param {string} channelId - Channel identifier used to scope the conversation.
+ * @param {string} role - Message role (e.g., "user" or "assistant").
+ * @param {string} content - Message text content.
+ * @param {string} [username] - Optional display name associated with the message.
+ * @param {string} [discordMessageId] - Optional native Discord message ID (used to construct jump URLs in the dashboard).
  */
-export function addToHistory(channelId, role, content, username) {
+export function addToHistory(channelId, role, content, username, discordMessageId) {
   if (!conversationHistory.has(channelId)) {
     conversationHistory.set(channelId, []);
   }
@@ -221,9 +223,9 @@ export function addToHistory(channelId, role, content, username) {
   if (pool) {
     pool
       .query(
-        `INSERT INTO conversations (channel_id, role, content, username)
-       VALUES ($1, $2, $3, $4)`,
-        [channelId, role, content, username || null],
+        `INSERT INTO conversations (channel_id, role, content, username, discord_message_id)
+       VALUES ($1, $2, $3, $4, $5)`,
+        [channelId, role, content, username || null, discordMessageId || null],
       )
       .catch((err) => {
         logError('Failed to persist message to DB', {

src/utils/logQuery.js

@@ -7,13 +7,10 @@
 
 import { getPool } from '../db.js';
 import { warn } from '../logger.js';
+import { escapeIlike } from './escapeIlike.js';
 
 const ALLOWED_LEVELS = ['error', 'warn', 'info', 'debug'];
 
-function escapeIlike(str) {
-  return str.replace(/[\\%_]/g, (ch) => `\\${ch}`);
-}
-
 /**
  * Query log entries from the PostgreSQL logs table.
  * Fails gracefully if the database is unavailable.

tests/api/routes/conversations.test.js

@@ -298,16 +298,10 @@ describe('conversations routes', () => {
 
     it('should return paginated conversations', async () => {
       const baseTime = new Date('2024-01-15T10:00:00Z');
+      // Mock returns rows in DESC order (newest first), matching ORDER BY created_at DESC.
+      // The route reverses them before grouping so the conversation anchor is still the oldest message.
       mockPool.query.mockResolvedValueOnce({
         rows: [
-          {
-            id: 1,
-            channel_id: 'ch1',
-            role: 'user',
-            content: 'Hello world',
-            username: 'alice',
-            created_at: baseTime.toISOString(),
-          },
           {
             id: 2,
             channel_id: 'ch1',
@@ -316,6 +310,14 @@ describe('conversations routes', () => {
             username: 'bot',
             created_at: new Date(baseTime.getTime() + 60000).toISOString(),
           },
+          {
+            id: 1,
+            channel_id: 'ch1',
+            role: 'user',
+            content: 'Hello world',
+            username: 'alice',
+            created_at: baseTime.toISOString(),
+          },
         ],
       });
 

tests/modules/ai.coverage.test.js

@@ -185,6 +185,7 @@ describe('ai module coverage', () => {
         'user',
         'hello',
         'testuser',
+        null,
       ]);
     });
 

tests/modules/ai.test.js

@@ -154,6 +154,7 @@ describe('ai module', () => {
         'user',
         'hello',
         'testuser',
+        null,
       ]);
     });
   });

tests/utils/escapeIlike.test.js

@@ -0,0 +1,82 @@
+import { describe, expect, it } from 'vitest';
+import { escapeIlike } from '../../src/utils/escapeIlike.js';
+
+describe('escapeIlike', () => {
+  describe('no special characters', () => {
+    it('returns plain strings unchanged', () => {
+      expect(escapeIlike('hello')).toBe('hello');
+      expect(escapeIlike('foo bar')).toBe('foo bar');
+      expect(escapeIlike('')).toBe('');
+    });
+
+    it('leaves alphanumeric and punctuation untouched', () => {
+      expect(escapeIlike('abc123')).toBe('abc123');
+      expect(escapeIlike('hello.world!')).toBe('hello.world!');
+      expect(escapeIlike('[email protected]')).toBe('[email protected]');
+    });
+  });
+
+  describe('percent sign (%)', () => {
+    it('escapes a single percent', () => {
+      expect(escapeIlike('%')).toBe('\\%');
+    });
+
+    it('escapes percent at the start', () => {
+      expect(escapeIlike('%foo')).toBe('\\%foo');
+    });
+
+    it('escapes percent at the end', () => {
+      expect(escapeIlike('foo%')).toBe('foo\\%');
+    });
+
+    it('escapes multiple percents', () => {
+      expect(escapeIlike('100%% done')).toBe('100\\%\\% done');
+    });
+  });
+
+  describe('underscore (_)', () => {
+    it('escapes a single underscore', () => {
+      expect(escapeIlike('_')).toBe('\\_');
+    });
+
+    it('escapes underscore in a word', () => {
+      expect(escapeIlike('snake_case')).toBe('snake\\_case');
+    });
+
+    it('escapes multiple underscores', () => {
+      expect(escapeIlike('__private__')).toBe('\\_\\_private\\_\\_');
+    });
+  });
+
+  describe('backslash (\\)', () => {
+    it('escapes a single backslash', () => {
+      expect(escapeIlike('\\')).toBe('\\\\');
+    });
+
+    it('escapes backslash in a path', () => {
+      expect(escapeIlike('C:\\Users')).toBe('C:\\\\Users');
+    });
+
+    it('escapes multiple backslashes', () => {
+      expect(escapeIlike('\\\\')).toBe('\\\\\\\\');
+    });
+  });
+
+  describe('combinations', () => {
+    it('escapes all three special characters together', () => {
+      expect(escapeIlike('%_\\')).toBe('\\%\\_\\\\');
+    });
+
+    it('escapes a realistic search pattern', () => {
+      expect(escapeIlike('50% off_sale\\')).toBe('50\\% off\\_sale\\\\');
+    });
+
+    it('escapes repeated mixed specials', () => {
+      expect(escapeIlike('%%__\\\\')).toBe('\\%\\%\\_\\_\\\\\\\\');
+    });
+
+    it('handles adjacent special chars with regular chars', () => {
+      expect(escapeIlike('a%b_c\\d')).toBe('a\\%b\\_c\\\\d');
+    });
+  });
+});