Skip to content

feat: webhook mode for production, polling for dev/test#36

Open
T0ha wants to merge 3 commits into
mainfrom
feature/telegram-markdown
Open

feat: webhook mode for production, polling for dev/test#36
T0ha wants to merge 3 commits into
mainfrom
feature/telegram-markdown

Conversation

@T0ha

@T0ha T0ha commented May 1, 2026

Copy link
Copy Markdown
Owner

Summary

  • Extract shared Telegram bot logic into Bodhi.TgUpdateHandler, replacing Bodhi.TgWebhookHandler
  • Add Bodhi.TgPollingHandler (thin wrapper for dev/test) and Bodhi.TgHookHandler (thin wrapper for production webhook mode)
  • Dynamic handler selection in Bodhi.Application based on :tg_mode config (:polling, :webhook, or :disabled)
  • Production webhook configured via TG_WEBHOOK_URL, TG_WEBHOOK_SECRET, TG_WEBHOOK_PORT env vars in runtime.exs

Test plan

  • mix compile — no warnings
  • mix test — 250 tests, 0 failures
  • mix format — clean
  • mix dialyzer — 0 errors
  • Verify polling starts in dev with iex -S mix
  • Verify webhook mode activates in production with correct env vars

🤖 Generated with Claude Code

Split TgWebhookHandler into three modules:
- TgUpdateHandler: shared business logic
- TgPollingHandler: thin polling wrapper for dev
- TgHookHandler: thin webhook wrapper for production

Handler selection is driven by :tg_mode config (:polling/:webhook/:disabled).

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@claude

claude Bot commented May 1, 2026

Copy link
Copy Markdown

Code Review

Overview

Clean refactor that separates transport concerns (webhook vs. polling) from shared bot logic. The three-way :tg_mode split (:webhook / :polling / :disabled) is well-designed and the Enum.reject(children, &is_nil/1) trick for conditional supervisor children is idiomatic.


Issues

Bug / Crash Risk — TgHookHandler.on_boot/0

{:ok, true} = Telegex.delete_webhook()
{:ok, true} = Telegex.set_webhook(env_config[:webhook_url], ...)

Bare pattern-matching with = will raise MatchError if either Telegram API call returns an error (network issue, bad token, rate-limit). That crashes the supervisor child on boot and, depending on restart strategy, could take down the whole app.

Suggestion: Use with and log or re-raise with a descriptive message:

def on_boot do
  env_config = Application.get_env(:bodhi, __MODULE__, [])

  with {:ok, true} <- Telegex.delete_webhook(),
       {:ok, true} <- Telegex.set_webhook(env_config[:webhook_url],
                        secret_token: env_config[:secret_token]) do
    %Telegex.Hook.Config{server_port: env_config[:server_port]}
  else
    error ->
      raise "Failed to configure Telegram webhook: #{inspect(error)}"
  end
end

Security — TG_WEBHOOK_SECRET is optional

In runtime.exs, TG_WEBHOOK_URL is required but TG_WEBHOOK_SECRET silently defaults to nil:

secret_token: System.get_env("TG_WEBHOOK_SECRET"),  # can be nil

Without a secret token, any actor who discovers the webhook URL can forge updates. For production this should either be required (with || raise) or at least emit a loud warning at startup:

secret_token =
  System.get_env("TG_WEBHOOK_SECRET") ||
    raise "environment variable TG_WEBHOOK_SECRET is missing"

Stale File Path in AGENTS.md

The module was renamed but the file path was not updated:

- **`Bodhi.TgUpdateHandler`** (`lib/bodhi/tg_webhook_handler.ex`) - Updated at line ~130

Should be lib/bodhi/tg_update_handler.ex.


Unintended Dev Port Change

config/dev.exs changes the Phoenix endpoint port from 40004003. This looks like a local development leftover that shouldn't be in this PR.


Missing server_port Fallback in TgHookHandler

If TG_WEBHOOK_PORT is not set, env_config[:server_port] is nil (the String.to_integer default only applies when the env var is absent — wait, actually it does default to "4001" in runtime.exs, so this is only a concern if TgHookHandler is somehow started without going through runtime.exs). Low risk, but worth a note or a module-level @default_port 4001.


Minor Notes

  • TgPollingHandler and TgHookHandler have no tests. Since they are thin wrappers the risk is low, but a smoke test asserting on_boot/0 returns the right struct type would give future regressions a clear signal.
  • The 80-character line-wrap refactoring across tg_update_handler.ex is good cleanup.
  • The new @spec for send_message/3 is a welcome addition.
  • The :disabled mode for test is elegant — no need to mock the handler at all.

Summary

Architecture ✅ Clean separation of transport vs. business logic
Correctness ⚠️ on_boot crash risk; optional webhook secret
Style ⚠️ Stale file path in AGENTS.md; dev port change leaking in
Tests ✅ Existing tests correctly updated; thin wrappers untested (low risk)
Security ⚠️ Webhook secret should be required in production

The two blocking items are the crash risk in on_boot and the optional webhook secret. Everything else is minor polish.

🤖 Generated with Claude Code

@T0ha

T0ha commented May 1, 2026

Copy link
Copy Markdown
Owner Author

Could we integrate webhook into phoenix routes without separate port?

- Replace Telegex.Hook.GenHandler with a GenServer that registers
  the webhook on boot + a Phoenix controller that receives updates
- Require TG_WEBHOOK_SECRET in production (was silently optional)
- Use `with` in webhook setup to handle API errors gracefully
- Fix stale file path in AGENTS.md
- Revert accidental dev port change (4003 → 4000)
- Remove separate webhook port config (TG_WEBHOOK_PORT)

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Comment thread config/runtime.exs Outdated
config :bodhi, :tg_mode, :webhook

webhook_url =
System.get_env("TG_WEBHOOK_URL") ||

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

Does this still needed or could be determined from Phoenix settings?

config =
Application.get_env(:bodhi, Bodhi.TgHookHandler, [])

if authorized?(conn, config[:secret_token]) do

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

This should be plug

Telegex.Type.Update
)

Task.start(fn ->

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

Async task is not needed here

- Extract secret-token check into BodhiWeb.Plugs.TelegramWebhookAuth and
  pipe the webhook scope through it instead of branching in the action
- Drop Task.start from controller; the request process already handles
  the update synchronously
- Derive webhook URL from BodhiWeb.Endpoint.url() + route path; remove
  TG_WEBHOOK_URL env var (TG_WEBHOOK_SECRET still required in prod)

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.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