Skip to content

Conversation

@zerob13
Copy link
Collaborator

@zerob13 zerob13 commented Feb 9, 2026

  • telegram
  • discord
  • confirmo.love
  • custom hooks

Summary by CodeRabbit

  • New Features

    • Notifications & Hooks settings UI to configure Telegram, Discord, Confirmo, and per-event custom commands; per-channel toggles, masking, per-event selection, persistence, and live test actions with detailed results.
    • Non-blocking lifecycle notifications emitted for SessionStart, UserPromptSubmit, PreToolUse, PostToolUse, PostToolUseFailure, PermissionRequest, Stop, and SessionEnd; dispatch is fault-tolerant, queued, and logged.
  • Tests

    • Unit tests for config normalization, retry parsing, and text utilities.
  • Documentation

    • Added detailed spec and phased delivery plan for hooks/webhook notifications.

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Feb 9, 2026

📝 Walkthrough

Walkthrough

Adds a hooks & webhook notifications system: shared types, config schemas and normalization, a HooksNotificationsService with dispatching/formatting/retries/command execution, presenter integration and non‑blocking lifecycle dispatch points, settings UI and i18n, and unit tests for core utilities.

Changes

Cohort / File(s) Summary
Specs / Docs
docs/specs/hooks-notifications/plan.md, docs/specs/hooks-notifications/spec.md
New plan and detailed spec describing scope, events, payloads, phased delivery, reliability, testing, and operational guidance.
Shared types
src/shared/hooksNotifications.ts, src/shared/types/index.d.ts, src/shared/types/presenters/legacy.presenters.d.ts
Adds hook event constants/types, config/payload/result types, re-exports types and extends presenter type surface for hooks APIs.
Config validation
src/main/presenter/hooksNotifications/config.ts
New Zod schemas, normalization utilities, sanitization and default factory for hooksNotifications settings.
Service implementation
src/main/presenter/hooksNotifications/index.ts
Implements HooksNotificationsService: payload builders, redaction/truncation, command execution, Telegram/Discord/Confirmo integrations, retry/429 handling, serial queueing, and test helpers.
Presenter integration
src/main/presenter/configPresenter/index.ts, src/main/presenter/index.ts
Adds hooksNotifications to IAppSettings, getter/setter/test methods on ConfigPresenter, Confirmo status helper, and registers HooksNotificationsService on Presenter.
Event dispatch points
src/main/presenter/agentPresenter/index.ts, src/main/presenter/agentPresenter/loop/toolCallProcessor.ts, src/main/presenter/agentPresenter/loop/agentLoopHandler.ts, src/main/presenter/agentPresenter/streaming/llmEventHandler.ts, src/main/presenter/agentPresenter/streaming/streamGenerationHandler.ts
Injects non-blocking dispatches for SessionStart, UserPromptSubmit, PreToolUse, PostToolUse, PostToolUseFailure, PermissionRequest, Stop, SessionEnd; propagates providerId into tool calls and introduces LLM error snapshots for enriched stop events.
Renderer: settings UI & route
src/renderer/settings/components/NotificationsHooksSettings.vue, src/renderer/settings/main.ts
New Vue settings component and route with per-channel controls, masked fields, per-event command configs, and per-channel testing UI.
i18n
many src/renderer/src/i18n/*/{routes.json,settings.json}
Adds route label and full notificationsHooks translations across locales (en-US, zh-CN, da-DK, fa-IR, fr-FR, he-IL, ja-JP, ko-KR, pt-BR, ru-RU, zh-HK, zh-TW, etc.).
Tests
test/main/presenter/hooksNotifications.test.ts
Unit tests for truncateText, parseRetryAfterMs, and normalizeHooksNotificationsConfig (validation and defaults).

Sequence Diagram(s)

sequenceDiagram
    participant User as User
    participant Presenter as AgentPresenter
    participant HooksSvc as HooksNotificationsService
    participant Config as ConfigPresenter
    participant Channel as External Channel

    User->>Presenter: submit prompt / tool call / session event
    Presenter->>HooksSvc: dispatchEvent(eventName, context)
    HooksSvc->>Config: getConfigSnapshot()
    Config-->>HooksSvc: HooksNotificationsSettings
    HooksSvc->>HooksSvc: build & redact payload
    alt command hook enabled
        HooksSvc->>HooksSvc: execute hook command (spawn, timeout, env)
        HooksSvc-->>HooksSvc: capture stdout/stderr
    end
    alt telegram/discord enabled
        HooksSvc->>HooksSvc: format (truncate, escape)
        HooksSvc->>Channel: POST notification
        Channel-->>HooksSvc: 200 or 429
        alt 429
            HooksSvc->>HooksSvc: parse Retry-After, queue & backoff
            HooksSvc->>Channel: retry POST
            Channel-->>HooksSvc: 200
        end
    end
    alt confirmo enabled
        HooksSvc->>HooksSvc: resolve confirmo path & execute
        HooksSvc-->>HooksSvc: capture result
    end
    HooksSvc-->>Presenter: return (non-blocking, logged)
    Presenter-->>User: continue flow
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~75 minutes

Possibly related PRs

  • ThinkInAIXYZ/deepchat#1276 — overlaps agentPresenter internals and streaming/tool-call lifecycle where hooks are injected.
  • ThinkInAIXYZ/deepchat#1241 — touches session/agent lifecycle that interacting hooks and dispatch points rely on.
  • ThinkInAIXYZ/deepchat#1208 — modifies tool-call flow and provider context propagation that this change extends.

Suggested labels

codex

🐰
I twitch my nose and tap the log,
Hooks hop softly through the fog.
Telegram chirps, Discord hums along,
Confirmo answers to the song.
Commands run quick — the system's strong!

🚥 Pre-merge checks | ✅ 2 | ❌ 1
❌ Failed checks (1 inconclusive)
Check name Status Explanation Resolution
Title check ❓ Inconclusive The title 'feat: add hooks for notify' is vague and incomplete. While it mentions 'hooks' and 'notify', it lacks specificity about the scope of changes including Telegram, Discord, Confirmo integrations, and lifecycle event dispatching. Consider revising the title to be more descriptive, such as 'feat: add webhook notifications and lifecycle hooks' or 'feat: add notification hooks (Telegram, Discord, Confirmo, custom)' to better convey the full scope of the implementation.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ 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 feat/notifaction

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

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

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 12

🤖 Fix all issues with AI agents
In `@docs/specs/hooks-notifications/spec.md`:
- Around line 159-161: Fix the typo in the environment variable names by
changing the `DEECHAT_` prefix to `DEEPCHAT_` where listed; update
`DEECHAT_HOOK_EVENT` → `DEEPCHAT_HOOK_EVENT`, `DEECHAT_CONVERSATION_ID` →
`DEEPCHAT_CONVERSATION_ID`, and `DEECHAT_WORKDIR` → `DEEPCHAT_WORKDIR` in the
spec (search for those identifiers to locate the entries).

In `@src/main/presenter/agentPresenter/streaming/llmEventHandler.ts`:
- Around line 543-562: The Stop and SessionEnd dispatches are currently inside a
single try/catch so a thrown error in
presenter.hooksNotifications.dispatchEvent('Stop', ...) prevents
presenter.hooksNotifications.dispatchEvent('SessionEnd', ...) from running;
split them into separate try/catch blocks (one around the 'Stop' dispatch and
another around the 'SessionEnd' dispatch) so each notification is guarded
independently, keep the existing warning logs for failures, and ensure the
existing finally behavior that calls this.errorByEventId.delete(eventId) still
executes after both attempts.
- Around line 153-202: The PreToolUse/PostToolUse/PostToolUseFailure hook
dispatches are gated to ACP-only in llmEventHandler (check the isAcpProvider
guard and dispatch code in llmEventHandler) but are dispatched unconditionally
in toolCallProcessor; make the behavior consistent by either applying the same
provider check to the presenter.hooksNotifications.dispatchEvent calls in
toolCallProcessor (guarding on state.message.model_provider === 'acp' or
equivalent provider check used by llmEventHandler) or by centralizing the
provider gating into a shared helper (e.g., shouldDispatchToolHooks(provider)
used by both llmEventHandler and toolCallProcessor) and updating all dispatch
sites (the dispatchEvent calls for 'PreToolUse', 'PostToolUse',
'PostToolUseFailure') to use that check so both streaming and MCP paths follow
the same provider restriction.

In `@src/main/presenter/agentPresenter/streaming/streamGenerationHandler.ts`:
- Around line 288-298: The SessionStart dispatch inside the continue-path is
missing the workdir field; update the payload sent by
presenter.hooksNotifications.dispatchEvent('SessionStart', {...}) to include
workdir: conversation.settings.agentWorkspacePath ?? null so its shape matches
the earlier SessionStart (and uses the available
conversation.settings.agentWorkspacePath from around line 248).

In `@src/main/presenter/configPresenter/index.ts`:
- Around line 1778-1796: The methods testTelegramNotification,
testDiscordNotification, testConfirmoNotification, testHookCommand (taking
HookEventName) and getConfirmoHookStatus currently access
presenter.hooksNotifications directly and can throw if the service is
uninitialized; update each to guard presenter and hooksNotifications (use
optional chaining like presenter?.hooksNotifications?.testTelegram() etc.) and
return a safe default when undefined (e.g., a HookTestResult indicating failure
or a { available: false, path: '' } object for getConfirmoHookStatus) or
alternatively perform an early check (if (!presenter?.hooksNotifications) throw
or return default) so callers never get a TypeError.

In `@src/main/presenter/hooksNotifications/index.ts`:
- Around line 377-384: In the env object constructed in const env:
Record<string, string> (inside hooksNotifications index), rename the
typo-prefixed environment variables from DEECHAT_* to DEEPCHAT_*: change
DEECHAT_HOOK_EVENT to DEEPCHAT_HOOK_EVENT, DEECHAT_CONVERSATION_ID to
DEEPCHAT_CONVERSATION_ID, and DEECHAT_WORKDIR to DEEPCHAT_WORKDIR so the
exported variables match the public contract; update the string keys where they
are set (including the conditional branches that use
payload.session.conversationId and payload.session.workdir) to the corrected
names.
- Around line 545-551: The code currently calls new URL(config.webhookUrl) which
can throw on malformed input; wrap URL construction in a validation step (either
a small isValidUrl check or a try/catch around new URL) inside the function that
builds/sends Discord payload (referencing buildDiscordEmbed and the sendDiscord
/ dispatchEventAsync call site) and if invalid return or throw a clear,
user-friendly Error (e.g. "Invalid Discord webhook URL") instead of allowing an
unhandled TypeError; ensure the error is propagated so dispatchEventAsync's
.catch() (and tests) receive the descriptive error.

In `@src/renderer/settings/components/NotificationsHooksSettings.vue`:
- Around line 136-138: The saving indicator span using isSaving is only rendered
inside the Telegram card; move or duplicate the indicator so saves from Discord,
Confirmo, or Commands show feedback: either relocate the <span v-if="isSaving"
class="text-xs text-muted-foreground">{{ t('common.saving') }}</span> to a
shared part of the NotificationsHooksSettings.vue template (e.g., next to the
page title/header) or add the same span to each card (Discord, Confirmo,
Commands, Telegram) so they all reference the same reactive isSaving property;
update any template sections that render card-specific actions (e.g., the
Discord/Confirmo/Commands card blocks) to include the indicator and ensure the
isSaving prop/computed used by the save handlers remains unchanged.

In `@src/renderer/src/i18n/en-US/settings.json`:
- Around line 67-120: Add the missing "notificationsHooks" translation block
(with the same nested keys as in en-US: title, description, events
{SessionStart, UserPromptSubmit, PreToolUse, PostToolUse, PostToolUseFailure,
PermissionRequest, Stop, SessionEnd}, telegram {...}, discord {...}, confirmo
{...}, commands {...}, test {...}) to each of the remaining locale JSON files
(da-DK, fa-IR, fr-FR, he-IL, ja-JP, ko-KR, pt-BR, ru-RU, zh-HK, zh-TW) so every
locale contains the "notificationsHooks" top-level key; copy the en-US structure
and placeholders and leave translated values as empty strings or same English
strings if translations are not yet available, ensuring valid JSON object syntax
in files that contain the keys.

In `@src/renderer/src/i18n/zh-CN/routes.json`:
- Around line 17-18: The new translation key "settings-notifications-hooks" was
added to en-US and zh-CN routes.json but is missing from the other locale
routes.json files; add the key "settings-notifications-hooks" to each of the
remaining locale routes.json files (da-DK, fa-IR, fr-FR, he-IL, ja-JP, ko-KR,
pt-BR, ru-RU, zh-HK, zh-TW) using the same key name and either the proper
localized string or a clear English placeholder copied from en-US until a
translation is available, ensuring consistent JSON formatting and placement
alongside neighboring keys like "settings-skills".

In `@src/renderer/src/i18n/zh-CN/settings.json`:
- Around line 67-120: You need to add the entire notificationsHooks translation
block (the object with keys notificationsHooks -> title, description, events
{...}, telegram {...}, discord {...}, confirmo {...}, commands {...}, test {...}
and all interpolation tokens like {path}, {ms}, {code}) to the 10 missing locale
files (da-DK, fa-IR, fr-FR, he-IL, ja-JP, ko-KR, pt-BR, ru-RU, zh-HK, zh-TW)
using the exact same key names so keys stay consistent across locales; if a
proper localized string is not available, copy the en-US text into those locales
as a temporary fallback but preserve placeholders and token formats (e.g.,
"{path}", "{ms}", "{code}"), and ensure the structure matches the
notificationsHooks object in zh-CN/en-US exactly.

In `@src/shared/hooksNotifications.ts`:
- Around line 108-114: The exported interface HookChannelResult is unused;
either remove it or document its intended future use: search for
HookChannelResult and confirm zero references, then if it's not needed delete
the exported interface declaration to avoid dead code (and update any
exports/index files if necessary), or if it's reserved for a planned Step 2, add
a concise comment above HookChannelResult explaining its purpose and why it is
separate from HookTestResult and HookCommandResult so future readers know it's
intentionally retained.
🧹 Nitpick comments (9)
src/main/presenter/index.ts (2)

113-113: Consider using an interface type instead of the concrete class.

All other presenter members are typed by their interface (e.g., IConfigPresenter, ISessionPresenter). Using the concrete HooksNotificationsService class here breaks that convention and tightens coupling.


332-344: hooksNotifications is not cleaned up in destroy().

The HooksNotificationsService holds internal SerialQueue instances. If any queued tasks are in-flight at shutdown, they won't be cancelled. Consider adding a destroy() method to the service and calling it here for clean shutdown, similar to other presenters.

src/main/presenter/agentPresenter/loop/toolCallProcessor.ts (1)

14-14: Direct singleton import breaks the dependency injection pattern.

ToolCallProcessor uses constructor injection for all other dependencies (ToolCallProcessorOptions), but presenter is imported as a module-level singleton. This makes the class harder to test in isolation and creates implicit coupling.

Consider passing hooksNotifications.dispatchEvent (or the service itself) through ToolCallProcessorOptions instead.

src/renderer/settings/main.ts (1)

85-94: Fractional position: 6.5 works but is fragile.

Consider renumbering positions to integers (e.g., bump skills to 8, notifications-hooks to 7) for cleaner ordering. Fractional positions can accumulate and become hard to manage.

src/main/presenter/hooksNotifications/config.ts (1)

178-202: Empty event arrays silently fall back to defaults — potential user surprise

On lines 185 and 191, if sanitizeEvents returns an empty array (e.g., all user-selected events were invalid, or the array was explicitly empty), the config silently reverts to DEFAULT_IMPORTANT_HOOK_EVENTS. This means a user who deliberately unchecks all events for Telegram/Discord will see them re-checked on reload.

This is likely benign since the UI always sends the current checked state, but it's worth documenting or considering [] as a valid "no events" selection:

-      events: telegramEvents.length ? telegramEvents : [...defaults.telegram.events]
+      events: telegram.events !== undefined ? telegramEvents : [...defaults.telegram.events]

This way, an explicitly provided (but empty) array is preserved, while a missing/undefined field gets defaults.

src/renderer/src/i18n/en-US/settings.json (1)

70-80: Event keys use PascalCase instead of lowercase.

The events keys (SessionStart, UserPromptSubmit, etc.) use PascalCase, deviating from the i18n guideline requiring "lowercase letters." This is likely intentional so keys match the HookEventName type values for programmatic lookup (e.g., t('notificationsHooks.events.' + eventName)), which is a reasonable trade-off. If so, a brief comment in the PR description noting this intentional deviation would be helpful.

As per coding guidelines: "Use dot-separated hierarchical structure for translation key naming with lowercase letters and descriptive names grouped by feature/context"

src/shared/hooksNotifications.ts (1)

14-20: Consider making DEFAULT_IMPORTANT_HOOK_EVENTS readonly.

The array is declared as a mutable HookEventName[]. Since it's used as a default template (spread into configs via [...DEFAULT_IMPORTANT_HOOK_EVENTS]), accidental mutation at runtime would corrupt all future defaults.

🛡️ Proposed fix
-export const DEFAULT_IMPORTANT_HOOK_EVENTS: HookEventName[] = [
+export const DEFAULT_IMPORTANT_HOOK_EVENTS: readonly HookEventName[] = [
   'SessionStart',
   'SessionEnd',
   'PostToolUseFailure',
   'PermissionRequest',
   'Stop'
-]
+] as const
src/main/presenter/hooksNotifications/index.ts (2)

70-93: parseRetryAfterMs seconds/milliseconds heuristic is fragile.

The logic parsed < 1000 ? Math.ceil(parsed * 1000) : Math.ceil(parsed) assumes values under 1000 are in seconds and values ≥ 1000 are already in milliseconds. Per RFC 7231, the Retry-After header is always in seconds, and Telegram's retry_after body field is also in seconds. A value of exactly 1000 seconds (unlikely but valid) would be misinterpreted as 1000ms instead of 1,000,000ms.

In practice, retry-after values from Telegram and Discord are small (< 60s), so this is unlikely to be hit. But a safer approach would be to always treat the value as seconds from known HTTP/API sources.

♻️ Proposed fix — always treat as seconds
  const header = response.headers.get('retry-after')
  if (header) {
    const parsed = Number(header)
    if (!Number.isNaN(parsed) && parsed > 0) {
-     return parsed < 1000 ? Math.ceil(parsed * 1000) : Math.ceil(parsed)
+     return Math.ceil(parsed * 1000)
    }
  }

  if (body && typeof body === 'object') {
    // ...
    if (retryAfter && retryAfter > 0) {
-     return retryAfter < 1000 ? Math.ceil(retryAfter * 1000) : Math.ceil(retryAfter)
+     return Math.ceil(retryAfter * 1000)
    }
  }

109-123: Redaction covers common patterns well.

The function handles explicit secrets, Discord webhook URLs, Telegram bot API URLs, and Authorization headers. The regex patterns are appropriately broad without being overly greedy.

One thought: the bot token used in the Telegram API URL (line 480) should also be included in the secrets list passed to redactSensitiveText when redacting command output, to ensure it's scrubbed from stdout/stderr if echoed. Currently, the secrets on line 435 only include conversationId and workdir.

Consider adding the Telegram bot token and Discord webhook URL to the secrets array in the close handler (line 435) when those channels are involved, or rely on the regex patterns already present (which already catch URL-based leaks). The regex coverage is likely sufficient.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 4

🤖 Fix all issues with AI agents
In `@src/renderer/src/i18n/da-DK/settings.json`:
- Line 1280: The "title" value currently set to "Meddelelser og Hooks" (JSON key
"title" in the Danish settings entry) is inconsistent with the route label used
in routes.json; change the string value to "Notifikationer og Hooks" so the
settings.json "title" matches the route label and remains consistent across the
UI.
- Line 1233: Replace the Danish translation value "Kroge Kommandoer" for the
JSON entry with a version that preserves the technical term "Hooks" and uses
lowercase for "kommandoer" (e.g., "Hooks kommandoer"); update the "title" value
at the corresponding JSON key so it reads "Hooks kommandoer" to follow Danish
technical conventions and grammar.
- Line 1237: Replace the translated Danish string for the product name with the
proper noun "Confirmo": locate the JSON entry where "title" currently equals
"Jeg bekræfter" in settings.json and set its value to "Confirmo" (keep casing
exact), so the product name remains untranslated per guidelines.

In `@src/renderer/src/i18n/ja-JP/routes.json`:
- Line 17: Replace the English label value for the "settings-skills" key with a
Japanese translation; locate the "settings-skills" entry in routes.json and
change its value from "Skills" to a localized string such as "スキル" or "スキル設定" to
match the other localized route labels.
🧹 Nitpick comments (1)
src/renderer/src/i18n/zh-TW/settings.json (1)

70-80: Event keys use PascalCase and should follow the lowercase guideline.

The coding guideline requires lowercase letters in i18n key names (e.g., common.button.submit). The event keys here (SessionStart, UserPromptSubmit, PreToolUse, etc.) use PascalCase instead and are consistent across all 12 locales. However, these keys are intentionally designed to match the event type names used in src/shared/hooksNotifications.ts and referenced directly in code via dispatchEvent() calls. If you prefer to enforce the lowercase convention, you would need to introduce a mapping layer between i18n keys and event type names in the hooks system.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 2

🤖 Fix all issues with AI agents
In `@src/main/presenter/agentPresenter/streaming/llmEventHandler.ts`:
- Around line 543-568: The two catch blocks after dispatching
presenter.hooksNotifications.dispatchEvent currently log the same misleading
message; update the first catch to warn about failures dispatching the "Stop"
hook and the second catch to warn about failures dispatching the "SessionEnd"
hook (keep the existing error variable and context), so the logs accurately
reflect whether presenter.hooksNotifications.dispatchEvent('Stop', ...) or
presenter.hooksNotifications.dispatchEvent('SessionEnd', ...) failed; leave the
surrounding finally that calls this.errorByEventId.delete(eventId) unchanged.

In `@src/main/presenter/hooksNotifications/index.ts`:
- Around line 454-468: When stdin.write throws inside the try/catch, ensure the
spawned child process is killed before finalizing: call child.kill() (or
child.kill('SIGKILL') for guaranteed termination) and close child.stdin if
necessary, then clearTimeout(timeout) and call finalize(...) as before; update
the catch block around the write in the code that references child, timeout,
start, stdout, stderr, DIAGNOSTIC_TEXT_LIMIT and finalize to perform the kill
and any cleanup before returning the error via finalize.
🧹 Nitpick comments (3)
src/main/presenter/hooksNotifications/index.ts (2)

546-552: Invalid URL throws instead of returning a structured HookTestResult.

For Telegram, missing config returns { success: false, ... } (line 476). For Discord, an invalid URL throws (line 551), which forces callers to distinguish between thrown errors and returned failure objects. Consider returning a result for consistency.

Proposed fix
    let url: URL
    try {
      url = new URL(config.webhookUrl)
    } catch {
-     throw new Error('Invalid Discord webhook URL')
+     return { success: false, durationMs: 0, error: 'Invalid Discord webhook URL' }
    }

70-93: parseRetryAfterMs should always treat retry values as seconds.

Per HTTP spec (RFC 9110), Telegram Bot API, and Discord API documentation, Retry-After and retry_after are always in seconds. The current < 1000 heuristic incorrectly assumes ambiguity between units: a legitimate value like 1200 (20 minutes) would be returned as 1200 ms instead of 1,200,000 ms.

Simplify both returns to always multiply by 1000:

Proposed fix
  const header = response.headers.get('retry-after')
  if (header) {
    const parsed = Number(header)
    if (!Number.isNaN(parsed) && parsed > 0) {
-     return parsed < 1000 ? Math.ceil(parsed * 1000) : Math.ceil(parsed)
+     return Math.ceil(parsed * 1000)
    }
  }

  if (body && typeof body === 'object') {
    // ...
    if (retryAfter && retryAfter > 0) {
-     return retryAfter < 1000 ? Math.ceil(retryAfter * 1000) : Math.ceil(retryAfter)
+     return Math.ceil(retryAfter * 1000)
    }
  }
src/renderer/settings/components/NotificationsHooksSettings.vue (1)

649-656: Unnecessary Promise.resolve().then(...) wrapper.

configPresenter.getConfirmoHookStatus() already returns a promise. The wrapper adds indirection without benefit.

Simplified version
    const [loadedConfig, status] = await Promise.all([
      configPresenter.getHooksNotificationsConfig(),
-     Promise.resolve()
-       .then(() => configPresenter.getConfirmoHookStatus())
-       .catch((error) => {
-         console.warn('Failed to load confirmo status:', error)
-         return null
-       })
+     configPresenter.getConfirmoHookStatus().catch((error) => {
+       console.warn('Failed to load confirmo status:', error)
+       return null
+     })
    ])

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🤖 Fix all issues with AI agents
In `@src/main/presenter/hooksNotifications/index.ts`:
- Around line 528-542: The returned HookTestResult may expose bot tokens/webhook
URLs because lastError is assigned raw JSON or exception messages in the
sendTelegram flow (variable lastError) and similarly in sendDiscord error
returns; before returning, run lastError through the existing
redactSensitiveText utility (or a new wrapper that redacts Telegram URL patterns
and Discord webhook URLs) so the HookTestResult.error contains a redacted
string; update the code paths that build the failure return in the Telegram
sender (lastError) and the Discord sender (sendDiscord error return) to use that
redaction function.
🧹 Nitpick comments (4)
src/main/presenter/hooksNotifications/index.ts (2)

70-93: Fragile seconds-vs-milliseconds heuristic in parseRetryAfterMs.

The HTTP Retry-After header and Telegram/Discord retry_after body fields are specified in seconds. The < 1000 threshold assumes any value ≥ 1000 is already in milliseconds, but a value of e.g. 1200 seconds would be misinterpreted as 1200 ms instead of 1,200,000 ms.

In practice, rate-limit delays are small, so this is unlikely to bite. But it would be more correct to always treat the value as seconds:

Suggested fix
-    if (!Number.isNaN(parsed) && parsed > 0) {
-      return parsed < 1000 ? Math.ceil(parsed * 1000) : Math.ceil(parsed)
-    }
+    if (!Number.isNaN(parsed) && parsed > 0) {
+      return Math.ceil(parsed * 1000)
+    }

Apply the same change to the body parsing branch (lines 87-88).


568-608: Duplicate retry-with-backoff logic between sendTelegram and sendDiscord.

The retry loop (fetch → check 429 → parse retry-after → wait → retry) is virtually identical in both methods. Consider extracting a shared fetchWithRetry helper to reduce duplication and ensure consistent behavior if the retry logic is updated later.

src/main/presenter/agentPresenter/streaming/llmEventHandler.ts (2)

393-417: Consider extracting the conversationId resolution fallback into a helper.

The pattern of resolving conversationId/providerId/modelId from messageManager.getMessage() when state is missing appears both here (lines 406–415) and in handleLLMAgentEnd (lines 532–541). Extracting a small private helper would reduce duplication.

Example helper
+ private async resolveEventContext(
+   eventId: string,
+   state?: GeneratingMessageState | null
+ ): Promise<{ conversationId?: string; providerId?: string; modelId?: string }> {
+   if (state) {
+     return {
+       conversationId: state.conversationId,
+       providerId: state.message.model_provider,
+       modelId: state.message.model_id
+     }
+   }
+   try {
+     const message = await this.messageManager.getMessage(eventId)
+     return {
+       conversationId: message.conversationId,
+       providerId: message.model_provider,
+       modelId: message.model_id
+     }
+   } catch {
+     return {}
+   }
+ }

543-568: Outer try without a catch is redundant.

The outer try { ... } finally { ... } on lines 543/566 only wraps two inner try/catch blocks that already handle their own exceptions. Since nothing can escape unhandled, the outer try/finally is purely for the errorByEventId.delete cleanup. This works correctly, but a simpler flat structure would be clearer:

Suggested simplification
-    try {
-      try {
-        presenter.hooksNotifications.dispatchEvent('Stop', {
-          conversationId,
-          providerId,
-          modelId,
-          stop: stopPayload
-        })
-      } catch (error) {
-        console.warn('[LLMEventHandler] Failed to dispatch Stop hook:', error)
-      }
-      try {
-        presenter.hooksNotifications.dispatchEvent('SessionEnd', {
-          conversationId,
-          providerId,
-          modelId,
-          stop: stopPayload,
-          usage,
-          error: errorInfo
-        })
-      } catch (error) {
-        console.warn('[LLMEventHandler] Failed to dispatch SessionEnd hook:', error)
-      }
-    } finally {
-      this.errorByEventId.delete(eventId)
-    }
+    try {
+      presenter.hooksNotifications.dispatchEvent('Stop', {
+        conversationId,
+        providerId,
+        modelId,
+        stop: stopPayload
+      })
+    } catch (error) {
+      console.warn('[LLMEventHandler] Failed to dispatch Stop hook:', error)
+    }
+    try {
+      presenter.hooksNotifications.dispatchEvent('SessionEnd', {
+        conversationId,
+        providerId,
+        modelId,
+        stop: stopPayload,
+        usage,
+        error: errorInfo
+      })
+    } catch (error) {
+      console.warn('[LLMEventHandler] Failed to dispatch SessionEnd hook:', error)
+    }
+    this.errorByEventId.delete(eventId)

@zerob13 zerob13 merged commit 2b7bb83 into dev Feb 9, 2026
2 checks passed
zerob13 added a commit that referenced this pull request Feb 9, 2026
* Merge pull request #1302 from ThinkInAIXYZ/bugfix/question-end-error

fix(agent): mark question tool messages sent

* refactor: route openai to responses (#1303)

* refactor: route openai to responses

* fix(i18n): localize openaiResponsesNotice

* feat: add hooks for notify (#1304)

* docs(hooks): add hooks notifications spec/plan

* docs(hooks): adjust settings UX and notifier triggers

* docs(hooks): define webhook notifications and lifecycle hooks

* feat(hooks): add confirmo notifications

* feat(hooks): improve notifications cards

* chore(i18n): add notifications hooks translations

* fix(hooks): correct env var prefix

* fix(hooks): align hook dispatch

* fix(settings): improve hooks ui

* fix(hooks): harden hook dispatch

* chore: update 0.5.8

---------

Co-authored-by: yyhhyyyyyy <yyhhyyyyyy8@gmail.com>
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.

1 participant