Bot Tools: draft/bot-tools + draft/bot-cmds (IRCv3 spec)#238
Open
ValwareIRC wants to merge 71 commits into
Open
Bot Tools: draft/bot-tools + draft/bot-cmds (IRCv3 spec)#238ValwareIRC wants to merge 71 commits into
ValwareIRC wants to merge 71 commits into
Conversation
Implements client-side support for the draft/ai-tools v0.4 spec: AI bots stream their workflow state (thinking, tool calls, results) as TAGMSG carrying a +obby.world/ai-tools JSON envelope; we render those workflows in a floating tray pinned to the top-right of the chat area, scoped to the current channel. Each workflow is its own card -- spinner + bot nick + dropdown chevron when collapsed, expands to show a Claude-Code-style timeline of steps (colored dot accent per type, content rendered in a monospace box for both string fragments and tool-call args). Multiple bots → multiple stacked cards, so workflow telemetry never eats into chat real-estate. Control signals (cancel / approve / reject) round-trip via TAGMSG to the bot's nick. Pending-approval steps surface inline buttons inside the card; the card header has a Stop button while running and a dismiss-X after the workflow reaches a terminal state. Includes: - aiTools.ts: decoder/encoder + TS types matching the spec - src/store/handlers/aiTools.ts: TAGMSG/CHANMSG/USERMSG → workflow state machine; string-content fragments concatenate so the ai-tools/content-stream batch reassembles correctly even when processed message-by-message - AiToolsCard + AiToolsTray: the floating UI - ChatArea wires the tray in, scoped to the selected target - draft/ai-tools added to the CAP REQ list - Tests: 21 new (decoder edge cases + handler state transitions) - Translations: all 18 supported locales
Replaces the flat JSON.stringify <pre> dump with a key→value chip tree. Primitives become inline colored chips (green strings, cyan numbers, purple bools); nested objects/arrays nest under a tinted left rule with their entries laid out one-per-row. Much easier to scan tool-call args once they get more than a couple of fields, which the user noticed when the bot started passing structured options through.
'Workflow history' (button title) and '{0} step(s)' (list row summary)
across all 18 non-English locales.
CollapsibleMessage renders as a block-level div, so the inline-flex pill stacked above the message body rather than sitting next to it. Float-left makes the body wrap around the chip, prepending it to the first line as intended.
Extends the workflow message schema with an optional `prompt` field that bots can use to ship a short truncated copy of the trigger message. The card renders it in italic muted text under the workflow name so users see what was asked without scrolling back to the trigger PRIVMSG. Field is optional -- bots that don't include it are unaffected, the card just omits the line. Plumbed end-to-end: lib/aiTools.ts decoder, AiWorkflow store type, applyWorkflowUpdate merge, AiToolsCard render.
Going from 60s to 5s means the previous 'fade only in the last 10s' logic would start with the card already half-transparent, so switch the opacity easing to span the full countdown (1.0 -> 0.15) instead.
Live workflows now claim a chat slot the moment the start TAGMSG lands. The slot shows the workflow name + a live preview of the latest step with a spinner, then morphs in place into the bot's final PRIVMSG (carrying the workflow pill) once it arrives -- no row jump. Historical (chathistory-replayed) workflows no longer pop floating tray cards on channel join; they remain in the workflow history popover and the inline pill on the original message.
…holder Two issues with the in-chat placeholder flow: 1. When the bot's tagged PRIVMSG morphs the placeholder, we were not resolving the +reply tag, so the morphed row had no reply quote block (whereas an untagged reply landed via the normal path with quote intact). Now resolveReplyMessage + extractMentions run on the morph path too. 2. If the bot's final PRIVMSG doesn't carry the workflow tag (only a terminal-state TAGMSG is sent), the morph path never fires and the placeholder is stuck on a "Thinking…" spinner while the bot's actual reply lands as a separate row. On terminal-state TAGMSG, remove the still-pending placeholder so the bot's reply can land normally with full reply context.
The tray filters out historical workflows so chathistory replay doesn't pop a wall of cards on channel join, but the inline pill's reopen action was leaving `historical` set -- meaning even after explicit click the card stayed filtered out and nothing happened. Clear it on reopen so an explicit view request always surfaces the card.
The pill used to sit in a flex column to the left of the body, which pushed the entire message content sideways and made the reply look visually off-axis from neighbouring messages. Strip it to a bare clickable icon (step count + name already live on the floating card and history popover) and absolutely-position it in the avatar gutter so the body keeps its natural alignment.
…g from count Two changes to how workflow steps are presented: 1. Display: a tool-call and its matching tool-result now render as a single row -- tool name in the header, then IN (call args) and OUT (result content) stacked, matching the Claude Code "Bash → Commit and push" pattern. Pairing is FIFO by tool name; an orphan result (no preceding call) still renders. Thinking / text rows are unchanged. 2. Counting: countableSteps() now powers the inline pill tooltip and the history popover label. It excludes thinking (model muttering, not work) and counts a tool-call/result pair as one step instead of two.
The auto-scroll effect only fired when the user was already at the bottom, which never happens on initial expand (scrollTop is 0). Track a "has-done-initial-scroll" flag per expansion: force-scroll to the bottom the first time the body mounts/expands, then fall back to the at-bottom check for subsequent step updates so a user scrolled-up to review earlier output isn't yanked away.
…check The previous approach checked "is the user at the bottom?" inside the content-update effect, but by then the new content had already grown scrollHeight -- the old scrollTop no longer qualified as bottom, so we never auto-scrolled even when the user was parked there. Track stickiness via the scroll event instead: whenever the user (or we) scrolls, recompute whether they're at the bottom and stash that into a ref. On content update, honour the flag. Reset to sticky on collapse so a fresh expansion always lands on the latest content.
Adds first-class support for PushBot-style slash commands as defined by
the obbyircd doc/pushbot-spec.md protocol. The user types /forecast
london in #weather and the client:
1. Looks up "forecast" in the per-server botCommands cache (keyed
by lowercased bot nick).
2. If a bot in the current channel exposes that command, builds a
base64-encoded JSON payload { name, options } and sends
@+draft/bot-cmd=<b64> TAGMSG <#channel|botnick>, picking the
PRIVMSG-style "public" or NOTICE-style "private" wire form based
on the command's visibility field.
3. Falls back to the raw IRC command path if no match -- existing
/op, /me etc. behaviour is preserved.
Discovery is event-driven:
* draft/bot-cmds added to ourCaps so the server knows we're aware
of the protocol (informational; the server still does the work).
* registerPushBotHandlers (new src/store/handlers/pushbot.ts) hooks
TAGMSG, decodes +draft/bot-cmds responses, and writes to
server.botCommands. A +draft/bot-cmds-changed broadcast clears
the cached entry so the next slash invocation triggers a refetch.
* The handler is wired into registerAllHandlers in
src/store/handlers/index.ts.
Types extended:
* Server.botCommands: Record<botNick, BotCommand[]> on src/types.
* BotCommand / BotCommandOption mirror the JSON the bot publishes
(name, description, visibility, scopes, options[]).
Resolution order in tryDispatchBotCommand mirrors §7.5 of the spec:
explicit /cmd@botnick targets first, then channel-bots, then DM
partners, then server-wide bots. Public invocations go to the
channel (everyone sees the reply); private ones go to the bot with
+draft/channel-context to keep whisper-style replies routed to the
right view.
Tests: tests/store/pushbot.test.ts covers cache population,
+draft/bot-cmds-changed invalidation, and that unrelated TAGMSGs are
ignored. All 60 test files (789 tests) still pass.
After RPL_ENDOFWHO (315) for a channel, scan the channel's user list for users marked isBot=true (set by handleWhoxReply when the +B mode flag is present) and send a +draft/bot-cmds-query TAGMSG to each that we don't already have cached. Result: by the time the user types '/' in a channel with bots in it, the autocomplete list is already populated.
Two visible-to-the-user gaps now closed: 1. ChatArea rendered the slash popover from `cmdsAvailable` only. Merge in command names from `server.botCommands` so /forecast shows up in the popover when the user is in a channel with a bot that has registered it. 2. Discovery used to fire only on WHO_END. Add a lazy `queryUncachedBotsInChannel` triggered the first time the user starts a slash command in a channel that has +B users without cached schemas, so the popover catches up if WHO completed before the handler attached.
Previously the popover rendered every suggestion as just `/name`; the
caller didn't know whether the source was a server-side built-in or a
PushBot, and there was no signal that /forecast even takes arguments.
Now:
* `SlashSuggestion` carries name + description + options[] + source
({kind:"builtin"} or {kind:"bot", botNick, scope:"channel"|"server"}).
ChatArea builds these from cmdsAvailable (builtins) and from
server.botCommands (PushBot schemas).
* The popover renders the bare name plus a "channel-bot" / "server-bot"
badge with the bot nick, the description below, and `<required>` /
`[optional]` placeholders inline next to the name. Channel-bots
only appear when their bot is a member of the active channel;
server-wide bots show up everywhere we have a cached schema.
* A new SlashParamHint floats above the input once the user has typed
past the command name -- it highlights the active argument (the one
the cursor is currently in), shows the param's type, "required"
tag, description, and any `choices` list. Disappears for builtins
(no schema) and once the user has scrolled past all declared opts.
Tests: 6 new cases for getActiveParamContext covering cmd-name
typing, `//foo` escape, position 0..N argument tracking, `/cmd@bot`
targeting suffix, and case-insensitive cmd matching. All 61 test
files (795 tests) green.
The popover was missing the React-side commands (/me, /msg, /nick, /whisper, /join, /part, /away, /back) because they're handled locally before they touch the wire and never appear in the obsidianirc/cmdslist set. Centralized them in src/lib/clientCommands.ts with full schemas (description + options) so the popover and the param-hint render them identically to PushBot commands. Source kinds now distinguish: * client → handled locally; slate badge, "(handled by ObsidianIRC)" * server → from obsidianirc/cmdslist; emerald badge * bot → draft/bot-cmds; amber "channel-bot" or purple "server-bot" Dedup is client > server > bot, so /me always renders as client even if the server's cmdslist also advertises it. Badges have hover-tooltips explaining the source.
Two passes squashed:
(1) Negotiate the obby.world/channel-bots capability, receive the
server's bot directory burst at welcome time (BATCH wrapper, one
TAGMSG per bot carrying obby.world/bot-info=<base64-json>) plus
incremental add/update/remove pushes. State lands in server.bots
(Record<lowerNick, PushBotInfo>) and mirrors into botCommands so
the slash popover stays warm without a separate
+draft/bot-cmds-query.
(2) New BotsModal — left pane: filter (All / Server-wide / Channel) +
search + scope/status badges and online dot; right pane: realname,
transport, joined channels, command schemas, IRCop action buttons
(Approve / Suspend / Unsuspend / Delete) for non-config-defined
bots. Wired into ChatHeader via onOpenBots, both as a desktop
icon button (🤖, hidden md:block) and as an overflow-menu entry
for narrow/mobile views -- the first cut only added the overflow
entry which is invisible on desktop widths.
Tests: 2 new vitest cases for the bot-info pipeline (add populates
server.bots + botCommands; remove clears both). 61 test files,
797 tests green.
…cons The first cut of BotsModal was a one-off custom layout (flex split, non-portal, ad-hoc styling) using a 🤖 emoji as a header decoration. That worked but it didn't match the rest of the app and looked off on mobile. Rework it to mirror UserSettings: * useMediaQuery to branch desktop vs mobile * useModalBehavior for escape / click-outside * desktop: backdrop + centered card (max-w-4xl, h-80vh) with a fixed- width sidebar (filterable bot list) and a content pane (selected bot detail); Discord-dark palette and discord-primary accents. * mobile: full-screen createPortal with two views (list → detail drill-in, back button to return), safe-area padding matching UserSettings. Icons: every emoji used as UI chrome now uses react-icons/fa. * 🤖 channel-header button → <FaRobot /> * 🤖 overflow-menu entry → <FaRobot /> * online indicator dot in the bot list → <FaCircle /> * empty-state placeholder → <FaRobot className="text-4xl" /> Same surface area: filter chips, search input, status/scope badges, IRCop action buttons (Approve / Suspend / Unsuspend / Delete) for non-config-defined bots. The 🤖 next to bot nicknames in chat is unchanged -- that's a pre-existing bot identity marker, not UI chrome.
…dslist Two bugs: 1) Server-scope bots (helpbot, dicebot) never showed up in the picker. The picker skipped any bot that wasn't a channel member, which was right for channel-scope bots but wrong for server-scope bots that never auto-join. Now: gate the membership check on the bot's scope; server-scope bots are always offered. 2) When a server's cmdsAvailable advertised a name (e.g. HELP) that a bot also defined (helpbot's /help), the server entry won the dedup set and the bot was shadowed; the hint code meanwhile pulled the bot's schema for that name, producing the "picker says it's the built-in but the hint reads like the bot" mismatch. Process bot commands before cmdsAvailable so the canonical bot owner wins the seen-set, and apply the same scope filter to the hint schemas. Drive-by: replace `choices!.length` with `(choices?.length ?? 0)` in SlashParamHint that fix:unsafe had downgraded to an unsafe optional chain on the comparator.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/lib/botTools.ts (2)
140-143:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winValidate
featuresagainst the allowed workflow feature set.
typeof f === "string"still accepts unsupported values and then casts them toAiWorkflowFeature[], sodecodeBotToolsValue()can return impossible feature flags even though this decoder is supposed to reject schema mismatches. Filter against aWORKFLOW_FEATURESset here, or returnnullif you want strict rejection.[suggested fix]
Diff
+const WORKFLOW_FEATURES: ReadonlySet<AiWorkflowFeature> = new Set([ + "interactive", + "reasoning", + "approval", +]); + ... - if (Array.isArray(obj.features)) - m.features = obj.features.filter( - (f): f is AiWorkflowFeature => typeof f === "string", - ) as AiWorkflowFeature[]; + if (Array.isArray(obj.features)) { + const features = obj.features.filter( + (f): f is AiWorkflowFeature => + typeof f === "string" && + WORKFLOW_FEATURES.has(f as AiWorkflowFeature), + ); + if (features.length !== obj.features.length) return null; + m.features = features; + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lib/botTools.ts` around lines 140 - 143, The current filter in decodeBotToolsValue that sets m.features from obj.features only checks typeof f === "string" and then casts to AiWorkflowFeature[], allowing unsupported feature strings; update the filtering to only accept values present in the canonical set (e.g., WORKFLOW_FEATURES) by checking membership before casting, or if you prefer strict decoding, return null from decodeBotToolsValue when any obj.features entry is not in WORKFLOW_FEATURES; target the m.features assignment and use AiWorkflowFeature and WORKFLOW_FEATURES symbols to implement the membership check or early null-return.
150-166:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winReject
tool-call/tool-resultframes that omittool.
countableSteps()pairs tool frames bytool, but the decoder currently accepts tool events without a tool name. That lets malformed payloads into state and can undercount by treatingundefined === undefinedas a match. Requireobj.toolwhentypeis"tool-call"or"tool-result".Diff
if ( typeof obj.wid !== "string" || typeof obj.sid !== "string" || typeof obj.type !== "string" || typeof obj.state !== "string" || !STEP_TYPES.has(obj.type as AiStepType) || !STEP_STATES.has(obj.state as AiStepState) ) return null; + if ( + (obj.type === "tool-call" || obj.type === "tool-result") && + typeof obj.tool !== "string" + ) { + return null; + } const m: AiStepMessage = {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lib/botTools.ts` around lines 150 - 166, The decoder that constructs an AiStepMessage currently allows frames with type "tool-call" or "tool-result" to omit obj.tool, which breaks countableSteps() pairing; update the validation in the decoder (the block that checks typeof obj.wid/sid/type/state and builds AiStepMessage) to additionally require typeof obj.tool === "string" and reject (return null) when obj.type is "tool-call" or "tool-result" and obj.tool is missing or not a string, and continue to set m.tool = obj.tool only after that check.
🧹 Nitpick comments (1)
src/lib/clientCommands.ts (1)
10-14: ⚡ Quick winDerive
CLIENT_COMMAND_NAMESfrom the same source asgetClientCommands().Lines 10-14 now understate the update path: the manual
CLIENT_COMMAND_NAMESset adds another sync point beyond the handler branch. If those lists drift, the popover/hint paths that treatgetClientCommands()as canonical will diverge from anyCLIENT_COMMAND_NAMESconsumer. Prefer one locale-independent source of truth for names/scope and derive both exports from it.Also applies to: 124-134
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lib/clientCommands.ts` around lines 10 - 14, Replace the duplicated manual name list with a single source of truth: keep the canonical array/object of client command descriptors (the data used by getClientCommands()) and derive CLIENT_COMMAND_NAMES by mapping that canonical list to its name strings (and similarly derive any other name-only exports around lines 124-134 from the same canonical list); update getClientCommands() to return or filter that canonical descriptor list rather than relying on a separately-maintained set, and remove the hard-coded CLIENT_COMMAND_NAMES so names and scopes always come from the same locale-independent source.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/components/layout/ChatArea.tsx`:
- Around line 951-969: The block that calls fetchUploadInfo(...) and mints
tokens (the tokens array, fetchUploadInfo, ircClient.requestToken and
waitForAuthToken inside the for loop) can throw before per-file try/catch runs;
wrap the entire initialization (the if block guarded by !tokenlessEndpoint &&
filehostUrl) in a try/catch so initialization failures are caught and handled
immediately: on error log it (or surface to the UI) and return early without
starting any per-file jobs, ensuring no unhandled rejection escapes; keep the
existing per-file logic unchanged but rely on the caught initialization error to
prevent further processing.
In `@src/components/ui/AddServerModal.tsx`:
- Around line 119-121: The current host-cleaning logic in AddServerModal
(cleanHost derived from serverHost) uses .replace(/[:/].*$/) which breaks IPv6
addresses; replace that logic by attempting to construct a URL from serverHost
and use url.hostname (which preserves IPv6 bracket notation and removes
port/path), and only fall back to the simple regex-based cleanup if URL parsing
throws; update the code that computes cleanHost in the AddServerModal component
to use this URL-parsing-first approach so ports/paths are stripped correctly
while IPv6 addresses like ::1 or [::1] remain intact.
In `@src/locales/fr/messages.po`:
- Around line 2727-2730: The French translation for the msgid "This server
doesn't support invite links (the<0>obby.world/invitation</0>capability isn't
advertised)..." has no spaces around the inline tag; update the msgstr so there
is a space before the opening tag and a space after the closing tag (i.e.,
change "...la capacité<0>obby.world/invitation</0>n'est..." to "...la capacité
<0>obby.world/invitation</0> n'est...") so the rendered UI text is not
concatenated.
---
Outside diff comments:
In `@src/lib/botTools.ts`:
- Around line 140-143: The current filter in decodeBotToolsValue that sets
m.features from obj.features only checks typeof f === "string" and then casts to
AiWorkflowFeature[], allowing unsupported feature strings; update the filtering
to only accept values present in the canonical set (e.g., WORKFLOW_FEATURES) by
checking membership before casting, or if you prefer strict decoding, return
null from decodeBotToolsValue when any obj.features entry is not in
WORKFLOW_FEATURES; target the m.features assignment and use AiWorkflowFeature
and WORKFLOW_FEATURES symbols to implement the membership check or early
null-return.
- Around line 150-166: The decoder that constructs an AiStepMessage currently
allows frames with type "tool-call" or "tool-result" to omit obj.tool, which
breaks countableSteps() pairing; update the validation in the decoder (the block
that checks typeof obj.wid/sid/type/state and builds AiStepMessage) to
additionally require typeof obj.tool === "string" and reject (return null) when
obj.type is "tool-call" or "tool-result" and obj.tool is missing or not a
string, and continue to set m.tool = obj.tool only after that check.
---
Nitpick comments:
In `@src/lib/clientCommands.ts`:
- Around line 10-14: Replace the duplicated manual name list with a single
source of truth: keep the canonical array/object of client command descriptors
(the data used by getClientCommands()) and derive CLIENT_COMMAND_NAMES by
mapping that canonical list to its name strings (and similarly derive any other
name-only exports around lines 124-134 from the same canonical list); update
getClientCommands() to return or filter that canonical descriptor list rather
than relying on a separately-maintained set, and remove the hard-coded
CLIENT_COMMAND_NAMES so names and scopes always come from the same
locale-independent source.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 99b137ca-58ff-4e85-bbfe-8f61545765ac
📒 Files selected for processing (61)
src/components/layout/ChatArea.tsxsrc/components/layout/ChatHeader.tsxsrc/components/message/BotInvocationChip.tsxsrc/components/message/BotToolsMessagePill.tsxsrc/components/message/BotToolsPlaceholderBody.tsxsrc/components/message/MessageItem.tsxsrc/components/ui/AddServerModal.tsxsrc/components/ui/BotToolsCard.tsxsrc/components/ui/BotToolsHistoryButton.tsxsrc/components/ui/BotToolsTray.tsxsrc/components/ui/BotsModal.tsxsrc/components/ui/SlashCommandPopover.tsxsrc/components/ui/SlashParamHint.tsxsrc/hooks/useMessageSending.tssrc/lib/botTools.tssrc/lib/clientCommands.tssrc/lib/irc/IRCClient.tssrc/locales/cs/messages.mjssrc/locales/cs/messages.posrc/locales/de/messages.mjssrc/locales/de/messages.posrc/locales/en/messages.mjssrc/locales/en/messages.posrc/locales/es/messages.mjssrc/locales/es/messages.posrc/locales/fi/messages.mjssrc/locales/fi/messages.posrc/locales/fr/messages.mjssrc/locales/fr/messages.posrc/locales/it/messages.mjssrc/locales/it/messages.posrc/locales/ja/messages.mjssrc/locales/ja/messages.posrc/locales/ko/messages.mjssrc/locales/ko/messages.posrc/locales/nl/messages.mjssrc/locales/nl/messages.posrc/locales/pl/messages.mjssrc/locales/pl/messages.posrc/locales/pt/messages.mjssrc/locales/pt/messages.posrc/locales/ro/messages.mjssrc/locales/ro/messages.posrc/locales/ru/messages.mjssrc/locales/ru/messages.posrc/locales/sv/messages.mjssrc/locales/sv/messages.posrc/locales/tr/messages.mjssrc/locales/tr/messages.posrc/locales/uk/messages.mjssrc/locales/uk/messages.posrc/locales/zh-TW/messages.mjssrc/locales/zh-TW/messages.posrc/locales/zh/messages.mjssrc/locales/zh/messages.posrc/store/handlers/botTools.tssrc/store/handlers/index.tssrc/store/index.tssrc/types/index.tstests/lib/botTools.test.tstests/store/botTools.test.ts
✅ Files skipped from review due to trivial changes (8)
- src/locales/cs/messages.mjs
- src/locales/es/messages.mjs
- src/locales/es/messages.po
- src/locales/it/messages.po
- src/locales/fi/messages.po
- src/locales/en/messages.po
- src/locales/cs/messages.po
- src/locales/de/messages.po
🚧 Files skipped from review as they are similar to previous changes (11)
- src/lib/irc/IRCClient.ts
- src/locales/en/messages.mjs
- src/locales/de/messages.mjs
- src/locales/it/messages.mjs
- src/components/message/BotInvocationChip.tsx
- src/locales/fr/messages.mjs
- src/components/ui/SlashCommandPopover.tsx
- src/locales/fi/messages.mjs
- src/components/ui/BotsModal.tsx
- src/components/ui/SlashParamHint.tsx
- src/hooks/useMessageSending.ts
🛑 Comments failed to post (3)
src/components/layout/ChatArea.tsx (1)
951-969:
⚠️ Potential issue | 🟠 Major | ⚡ Quick winCatch upload-initialization failures before starting jobs.
fetchUploadInfo/ token minting can throw before per-file upload try/catch runs. Right now that failure can escape UI handlers as an unhandled rejection and fail silently for users.🛠️ Proposed fix
- if (!tokenlessEndpoint && filehostUrl) { - info = await fetchUploadInfo(filehostUrl); - // Serialised because waitForAuthToken resolves on the first matching - // TOKEN_GENERATE event -- parallel mints would race for the reply. - const scope = target.startsWith("#") ? `channel:${target}` : undefined; - for (let i = 0; i < files.length; i++) { - ircClient.requestToken(selectedServerId, "filehost", scope); - const tok = await waitForAuthToken(selectedServerId, "filehost"); - if (!tok) { - console.error( - "draft/authtoken: server did not return a filehost token", - ); - return; - } - tokens.push(tok); - } - } + try { + if (!tokenlessEndpoint && filehostUrl) { + info = await fetchUploadInfo(filehostUrl); + // Serialised because waitForAuthToken resolves on the first matching + // TOKEN_GENERATE event -- parallel mints would race for the reply. + const scope = target.startsWith("#") ? `channel:${target}` : undefined; + for (let i = 0; i < files.length; i++) { + ircClient.requestToken(selectedServerId, "filehost", scope); + const tok = await waitForAuthToken(selectedServerId, "filehost"); + if (!tok) { + console.error( + "draft/authtoken: server did not return a filehost token", + ); + return; + } + tokens.push(tok); + } + } + } catch (err) { + console.error("File upload initialization failed:", err); + return; + }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.let info: Awaited<ReturnType<typeof fetchUploadInfo>> = null; const tokens: string[] = []; try { if (!tokenlessEndpoint && filehostUrl) { info = await fetchUploadInfo(filehostUrl); // Serialised because waitForAuthToken resolves on the first matching // TOKEN_GENERATE event -- parallel mints would race for the reply. const scope = target.startsWith("#") ? `channel:${target}` : undefined; for (let i = 0; i < files.length; i++) { ircClient.requestToken(selectedServerId, "filehost", scope); const tok = await waitForAuthToken(selectedServerId, "filehost"); if (!tok) { console.error( "draft/authtoken: server did not return a filehost token", ); return; } tokens.push(tok); } } } catch (err) { console.error("File upload initialization failed:", err); return; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/components/layout/ChatArea.tsx` around lines 951 - 969, The block that calls fetchUploadInfo(...) and mints tokens (the tokens array, fetchUploadInfo, ircClient.requestToken and waitForAuthToken inside the for loop) can throw before per-file try/catch runs; wrap the entire initialization (the if block guarded by !tokenlessEndpoint && filehostUrl) in a try/catch so initialization failures are caught and handled immediately: on error log it (or surface to the UI) and return early without starting any per-file jobs, ensuring no unhandled rejection escapes; keep the existing per-file logic unchanged but rely on the caught initialization error to prevent further processing.src/components/ui/AddServerModal.tsx (1)
119-121:
⚠️ Potential issue | 🟠 Major | ⚡ Quick winIPv6 address parsing is broken by the regex.
The regex
/[:/].*$/strips everything from the first:or/, which breaks IPv6 addresses:
[::1]→[(colon inside brackets is matched)::1→:(first colon is matched)[2001:db8::1]→[2001This will cause connection failures if users enter IPv6 addresses (e.g., localhost
::1for development).🔧 Proposed fix using URL parsing
Use URL parsing to extract the hostname, which correctly handles IPv6 bracket notation:
const port = Number.parseInt(serverPort, 10); - // Strip scheme AND any embedded :port / path so we don't end up - // appending port twice (e.g. ircs://host:6697:6697). - const cleanHost = serverHost - .replace(/^(https?|wss?|ircs?|irc):\/\//, "") - .replace(/[:/].*$/, ""); + // Strip scheme and extract hostname (handles IPv6, ports, and paths) + let cleanHost = serverHost.replace(/^(https?|wss?|ircs?|irc):\/\//, ""); + try { + // Use URL parsing to extract hostname (handles IPv6 brackets) + const url = new URL(`http://${cleanHost}`); + cleanHost = url.hostname; + } catch { + // Fall back to regex for plain hostnames without special chars + cleanHost = cleanHost.replace(/[:/].*$/, ""); + } finalHost = useWebSocket ? `wss://${cleanHost}:${port}` : `ircs://${cleanHost}:${port}`;This approach:
- Uses
URL.hostnamewhich correctly extracts IPv6 addresses from bracket notation- Falls back to the existing regex for simple hostnames if URL parsing fails
- Preserves the intent to strip ports and paths while fixing IPv6 support
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.const port = Number.parseInt(serverPort, 10); // Strip scheme and extract hostname (handles IPv6, ports, and paths) let cleanHost = serverHost.replace(/^(https?|wss?|ircs?|irc):\/\//, ""); try { // Use URL parsing to extract hostname (handles IPv6 brackets) const url = new URL(`http://${cleanHost}`); cleanHost = url.hostname; } catch { // Fall back to regex for plain hostnames without special chars cleanHost = cleanHost.replace(/[:/].*$/, ""); } finalHost = useWebSocket ? `wss://${cleanHost}:${port}` : `ircs://${cleanHost}:${port}`;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/components/ui/AddServerModal.tsx` around lines 119 - 121, The current host-cleaning logic in AddServerModal (cleanHost derived from serverHost) uses .replace(/[:/].*$/) which breaks IPv6 addresses; replace that logic by attempting to construct a URL from serverHost and use url.hostname (which preserves IPv6 bracket notation and removes port/path), and only fall back to the simple regex-based cleanup if URL parsing throws; update the code that computes cleanHost in the AddServerModal component to use this URL-parsing-first approach so ports/paths are stripped correctly while IPv6 addresses like ::1 or [::1] remain intact.src/locales/fr/messages.po (1)
2727-2730:
⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd spaces around the inline capability tag in the French translation.
Line 2729 currently concatenates words around
<0>...</0>, which will render awkwardly in UI text.💡 Suggested fix
-msgstr "Ce serveur ne prend pas en charge les liens d'invitation (la capacité<0>obby.world/invitation</0>n'est pas annoncée). Vous pouvez toujours discuter normalement ; ce panneau est destiné aux réseaux propulsés par obbyircd." +msgstr "Ce serveur ne prend pas en charge les liens d'invitation (la capacité <0>obby.world/invitation</0> n'est pas annoncée). Vous pouvez toujours discuter normalement ; ce panneau est destiné aux réseaux propulsés par obbyircd."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/locales/fr/messages.po` around lines 2727 - 2730, The French translation for the msgid "This server doesn't support invite links (the<0>obby.world/invitation</0>capability isn't advertised)..." has no spaces around the inline tag; update the msgstr so there is a space before the opening tag and a space after the closing tag (i.e., change "...la capacité<0>obby.world/invitation</0>n'est..." to "...la capacité <0>obby.world/invitation</0> n'est...") so the rendered UI text is not concatenated.
ValwareIRC
requested review from
matheusfillipe
and removed request for
matheusfillipe
…y rename - pushbot handler: on BATCH_START with type=draft/bot-cmds, allocate a fragments buffer keyed by (serverId, batchRef). Each in-batch TAGMSG with +draft/bot-cmds appends its raw base64 fragment to the buffer instead of decoding individually. On BATCH_END, concatenate, base64-decode, JSON-parse, and commit. Matches the bot-tools spec's 'split across batch messages, concatenated before decoding' shape. - Rename +obby.world/invoked-by -> +draft/invoked-by in the bot invocation chip and the MessageItem tag lookup. The spec now defines this tag under draft/.
…y bot The draft/bot-cmds spec uses BATCH +ref draft/bot-cmds <target>, where the target on the open line is the asker, not the sender. The sender nick lives in the BATCH command's source prefix; without exposing it on the BATCH_START event the pushbot handler stored each batch under an empty-string key and could never write the reassembled list back to the right bot's botCommands entry.
matheusfillipe
left a comment
matheusfillipe
left a comment
Contributor
There was a problem hiding this comment.
did a local review, flagging the few that actually worry me. the bot-cmd shadowing one is the important one.
| } | ||
| // server-wide bots (any bot we know that defines the command) | ||
| if (!matches.length) { | ||
| for (const [bot, list] of Object.entries(bots)) { |
Contributor
There was a problem hiding this comment.
security: this server-wide fallback lets a bot shadow real commands. /oper x y and /ns identify pass aren't builtins so they fall through to here before the raw send, and we match any bot anywhere on the network. so a bot registering oper/ns just grabs the user's password. we also cache botCommands per sender with no isBot check (commitBotCmds). think we should only dispatch on explicit /cmd@bot or isBot + in-channel, kill the server-wide fallback, and never let a bot name shadow a server/privileged command.
Contributor
Author
There was a problem hiding this comment.
I agree, these are something that needs to be addressed
| processedMessageIds, | ||
| }; | ||
| } | ||
| return { aiWorkflows, processedMessageIds }; |
Contributor
There was a problem hiding this comment.
i think we drop the bot's final answer here when there was no placeholder. on idx < 0 we still add the msgid to processedMessageIds and return without pushing a row, and since botTools runs before the message handler the normal CHANMSG path then dedupes it away. happens when a bot doesn't tag start or you join mid-workflow. on idx < 0 we should just append it as a normal message instead of swallowing it.
| )} | ||
|
|
||
| <div className="relative min-w-0"> | ||
| <BotInvocationChip tagValue={message.tags?.["+draft/invoked-by"]} /> |
Contributor
There was a problem hiding this comment.
this renders for any message carrying +draft/invoked-by, and it's a + client tag so anyone can forge it. i can send a normal line tagged invoked-by={nick:"someadmin",name:"ban"...} and everyone in the channel sees 'someadmin ran /ban ...'. should only show it for real bot senders (we already have isBot right here) and not trust the nick blindly.
Contributor
Author
There was a problem hiding this comment.
I'd like to enforce this server-side as well as client-side
MessageItem.tsx branches off into ActionMessage.tsx for any message whose content starts with \\u0001ACTION, and that path was rendered without the chip -- so every bot whose reply was a /me (e.g. 8ball's 'Cloudia shakes the magic 8 ball...') showed up with no attribution even when +draft/invoked-by was on the wire. The non-action PRIVMSG branch in MessageItem.tsx already rendered it; mirror that import + placement in ActionMessage.tsx above the italic body so the user sees who triggered the action and with what args.
matheusfillipe
left a comment
matheusfillipe
left a comment
Contributor
There was a problem hiding this comment.
few more after a closer look at the batch path 🙏 mostly questions
| ) | ||
| .map((c) => c.name); | ||
| const existingBot = s.bots?.[key]; | ||
| const botsNext = existingBot |
Contributor
There was a problem hiding this comment.
hmm doesnt this undo the isBot gate we just added in commitBotCmds? we synth the sender into s.bots here before checking anything, so by the time BATCH_END runs commitBotCmds the knownBot check already passes for them. so any user opening a draft/bot-cmds batch gets added as an active bot and their cmds accepted? think we'd want the same isBot check here too
| ircClient.on("BATCH_START", ({ serverId, batchId, type, sender }) => { | ||
| if (type !== "draft/bot-cmds") return; | ||
| console.log("[bot-cmds] BATCH start", { batchId, sender }); | ||
| botCmdsBatches.set(`${serverId}:${batchId}`, { |
Contributor
There was a problem hiding this comment.
if BATCH_END never comes (bot opens it and never closes, or just keeps streaming chunks) dont we leak here? the map entry + fragments grow forever and the sender stays stuck with the loading spinner in the modal. maybe a cap or timeout?
|
|
||
| ircClient.on("BATCH_START", ({ serverId, batchId, type, sender }) => { | ||
| if (type !== "draft/bot-cmds") return; | ||
| console.log("[bot-cmds] BATCH start", { batchId, sender }); |
Contributor
There was a problem hiding this comment.
leftover debug logs? these + the ones at BATCH end / decoded ok. want to drop before merge i think
| )} | ||
|
|
||
| <div className="relative min-w-0"> | ||
| {isBot && ( |
Contributor
There was a problem hiding this comment.
nice. one thing — isBot here also trusts metadata.bot, and if the network lets a user set their own bot metadata key cant they still forge the chip? the WHO +B flag / bot message tag are server-set so those are solid, maybe just drop the metadata source for this check?
Contributor
Author
There was a problem hiding this comment.
The network also lets bots/users mark themselves as bots with +B mode, there really is no protection against a user making this forge, but I imagine in a real world scenario where a channel bot is trusted enough (by the channel staff) to be able to take meaningful actions like kicking or banning, it should be trusted enough to report the correct invoked-by. That said, plain users can do a similar things already (/part #lobby kicked by matheusfillipe (get out of here, butt)) which look fairly convincing.
I'd say if it became a problem, we can import my pre-existing module which allows channel operators to restrict usage of a specific message-tag, then channel ops are able to completely forbid or allow which channel users are able to use invoked-by beyond the existing measurements we've taken.
|
|
||
| // IRC tag-value escape — applied just before putting the value on the | ||
| // wire. Mirrors the unescape in src/lib/ircUtils.tsx. | ||
| export function escapeIrcTagValue(s: string): string { |
Contributor
There was a problem hiding this comment.
is this used anywhere? i only see it in the test, and voice.ts already has its own copy. we base64 the payload before sending so i dont think we escape anywhere — dead code?
Contributor
Author
There was a problem hiding this comment.
This is used in other places indeed such as with unrealircd.org/json-log and webrtc signalling, this is the traditional escaper. I only used base64 because I'm not sure on the integrity of this function.
Edit: I mean while testing I ran into problems like the fact that some lines were being swallowed by the ircd on large and fast outputs from bots with large amounts of commands, and I was unsure if there was a problem with the unescaping and escaping over the wire, so I switched to b64 to try to side-step it.
matheusfillipe
previously approved these changes
Jun 11, 2026
matheusfillipe
left a comment
matheusfillipe
left a comment
Contributor
There was a problem hiding this comment.
overall lgtm. Left some comments you could quickly fix but feel free to merge afterwards. or after you finish making cloudbot work with this.
…o popover on chip
…low doesn't clip it
… text color Two cosmetic fixes on the flyout's 'Show in Bots Menu' row: - Sticky positioning was on the button itself, but `sticky` only pins the element's content box, not the gap consumed by the parent's vertical padding (`py-1` on the menu). Command rows scrolled visibly through the sliver below the button. Wrap the button in a sticky <div> with z-10 and put the bg + border on that wrapper so the entire pinned strip is opaque. - text-discord-blue rendered dark navy on the dark-300 background; the rest of the menu uses text-discord-text-normal with hover text-white. Switch to that pair for visual consistency.
…eader sits flush The submenu container had `py-1` -- 4px top + 4px bottom padding. sticky top-0 anchors the header below the top padding, so 4px of the first command row peeked through that transparent strip while scrolling. Swap `py-1` for `pb-1` so the header is flush with the top border and there's no peek-through; bottom still has its breathing room.
Biome's noArrayIndexKey lint flagged the pair-rendering map. All three paramPairs paths produce a non-empty p.key already (object entries use the property name; array entries use the index as the key field; the primitive/string path emits a single pair with an empty key, which has no sibling to collide with). Use that as the React key instead of the array index.
# Conflicts: # src/locales/cs/messages.mjs # src/locales/de/messages.mjs # src/locales/en/messages.mjs # src/locales/es/messages.mjs # src/locales/fi/messages.mjs # src/locales/fr/messages.mjs # src/locales/it/messages.mjs # src/locales/ja/messages.mjs # src/locales/ko/messages.mjs # src/locales/nl/messages.mjs # src/locales/pl/messages.mjs # src/locales/pt/messages.mjs # src/locales/ro/messages.mjs # src/locales/ru/messages.mjs # src/locales/sv/messages.mjs # src/locales/tr/messages.mjs # src/locales/uk/messages.mjs # src/locales/zh-TW/messages.mjs # src/locales/zh/messages.mjs
…adge maps Audit of PR #238 surface (workflow + bot-cmds UI) found 30 unwrapped English strings. All four scouts converged on two patterns: 1. Module-scope label maps (STATUS_BADGE / SCOPE_BADGE / FILTER_LABELS in BotsModal.tsx; badgeStyle() in SlashCommandPopover.tsx). These evaluate before i18n.activate so any t-template invoked at module init returns the source key, not the active locale's string. Move the colour-class records to module scope (locale-invariant) and wrap labels + tooltips in per-render hooks (useStatusBadge, useScopeBadge, useFilterLabels) / functions that take t as an argument (badgeStyle(source, t)). 2. Direct unwrapped literals: 'active', 'timed out', 'tool' (the fallback display name for an anonymous step in BotToolsPlaceholderBody), 'required', 'nick' / '#channel' placeholders in SlashCommandParamModal, 'Slash commands' header, 'Play Tic-Tac-Toe' in ChatHeader (label + aria-label + title), and the 'X seconds/minutes/hours/days' template literals in UserProfileModal's formatIdleTime. Also wraps the effective workflow state ('complete' / 'failed' / 'cancelled' / 'timed out') in BotToolsHistoryButton so the row's '· complete' style suffix doesn't leak English into non-English UIs. Tests/lint clean; catalogs recompiled via i18n:extract + i18n:compile. The diff includes the auto-generated locale .po + .mjs updates so the next merge into main doesn't reintroduce stale msgid drift.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Reworks and combines two previously-separate feature branches — the workflow viewer (
draft-ai-tools) and slash commands (feat/pushbot-client) — to the IRCv3 Bot Tools draft spec.draft/bot-tools (workflow transparency)
draft/bot-tools(wasdraft/ai-tools); client-only tag+draft/bot-tools(was the vendor+obby.world/ai-tools)src/lib/base64.ts) instead of escaped raw JSONthinking→reasoning; thesteeraction →input; added the workflowfeaturesarray (interactive/reasoning/approval) and stepcancelled-bydraft/bot-cmds (slash commands)
+draft/bot-cmds-query(was=1)contexts(public/private/pm) +requires, dropping the legacyvisibility/scopes/versionfieldsbot(public-channel disambiguation) and thechannel(private context) instead of relying on+draft/channel-context, which is not valid onTAGMSGThe obby.world bot-directory layer (
obby.world/channel-bots,obby.world/bot-info, the Bots management modal) is intentionally kept as a vendor feature — only its command display now readsdraft/bot-cmds.Testing
npm run test— 818 passing, 1 skippednpm run build— cleanServer side
Requires the companion obbyircd changes on
unreal60_dev(commit11d5d6d76): thedraft/bot-tools/draft/bot-cmdscapabilities, schema normalisation,+draft/bot-cmd-error, and the legacy-compatibility bridge.Summary by CodeRabbit