Conversation
WalkthroughA single comment line was inserted in SlackBot.py between the history list initialization and the append_history function. No code logic or public interfaces were changed. Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~2 minutes Pre-merge checks (2 passed, 1 warning)❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
Poem
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. ✨ Finishing touches
🧪 Generate unit tests
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (7)
SlackBot.py (7)
21-25: History cap off-by-one; switch to deque(maxlen=20).Current logic allows 21 items when len == 20. Use deque to enforce the bound cleanly.
-import time +import time +from collections import deque @@ -history = [] +history = deque(maxlen=20) @@ -def append_history(t): - if len(history) > 20: - del history[:1] - history.append(t) - return True +def append_history(t): + history.append(t) + return TrueIf you must keep list, use
while len(history) >= 20: del history[0]before append.
6-11: Guard missing BOT_ID to avoid TypeError at import time.If BOT_ID is unset,
"<@" + BOT_ID + ">"raises. Fail fast with a clear error.-# testbot's ID as an environment variable -BOT_ID = os.environ.get("BOT_ID") -# constants -AT_BOT = "<@" + BOT_ID + ">" +# testbot's ID as an environment variable +BOT_ID = os.environ.get("BOT_ID") +if not BOT_ID: + raise RuntimeError("BOT_ID environment variable is required") +# constants +AT_BOT = f"<@{BOT_ID}>"
87-95: Bug:tsis undefined in this scope.
handle_responseusests[0:10]but doesn't receivets. Pass it in and slice safely.-def handle_response(command, channel, user): +def handle_response(command, channel, user, ts): @@ - append_history({'time': ts[0:10], '__SESSION__': "initiated", 'user': user}) + append_history({'time': str(ts)[:10], '__SESSION__': "initiated", 'user': user})And at call site (Lines 136-140):
- handle_response(command, channel, user) + handle_response(command, channel, user, ts)
28-33: Makeis_user_in_listreturn a stable type.It returns a tuple on hit and None otherwise, causing double-calls and awkward truthiness checks.
-def is_user_in_list(u): - length = len(history) - for i in range(length): - if history[i]['user'] == u: - return i, True +def is_user_in_list(u): + for i, item in enumerate(history): + if item.get('user') == u: + return i + return -1Follow-ups: update call sites to compare against
-1and avoid re-calling it.
35-40: Deletion while iterating; break after remove.Index shifts after deletion; since users are unique, remove and break.
-def del_user(u): - length = len(history) - for i in range(length): - if history[i]['user'] == u: - del history[i] +def del_user(u): + for i, item in enumerate(list(history)): + if item.get('user') == u: + del history[i] + breakIf using deque,
del history[i]still works.
53-66: Return a boolean deterministically; avoid prints and double lookups.Function returns
TrueorNone, prints debug output, and callsis_user_in_listtwice.-def past_seconds(user, t): +def past_seconds(user, t): @@ - epoch = int(time.time()) - - if is_user_in_list(user): - index = is_user_in_list(user) - print(int(history[index[0]]['time']) + 300, epoch) - if int(history[index[0]]['time']) + t < epoch: - return True + epoch = int(time.time()) + idx = is_user_in_list(user) + if idx == -1: + return False + try: + last = int(history[idx]['time']) + except (KeyError, ValueError, TypeError): + return False + return (last + int(t)) < epoch
113-126: Parse text after mention as the comment says.The function claims to return text after @mention but currently returns full text.
- if 'text' in output and not_bot(output): - # return text after the @ mention, whitespace removed - return output['text'].strip().lower(), output['channel'], output['ts'], output['user'] + if 'text' in output and not_bot(output): + text = output['text'].strip() + if AT_BOT and text.startswith(AT_BOT): + text = text[len(AT_BOT):].strip() + return text.lower(), output['channel'], output['ts'], output['user']This makes
handle_help/handle_responsesimpler.
🧹 Nitpick comments (4)
SlackBot.py (4)
20-20: Remove meaningless comment or replace with useful context.The comment adds no value and can be removed to reduce noise.
-# there are no comments here +# Keeps a short per-user interaction history for throttling and session state.
75-78: Help trigger condition simplifies after proper parsing.With mention stripped, just check for “help”.
-def handle_help(command, channel): - if AT_BOT and 'help' in command: +def handle_help(command, channel): + if 'help' in command:Optionally require exact
command == 'help'to avoid accidental matches.
70-71: Add basic error handling to Slack API calls.Network/API errors will raise or return non-ok; avoid silent failures.
-def send_message(response, channel): - slack_client.api_call("chat.postMessage", channel=channel, text=response, as_user=True) +def send_message(response, channel): + try: + slack_client.api_call("chat.postMessage", channel=channel, text=response, as_user=True) + except Exception as e: + print(f"send_message failed: {e}")
130-145: RTM reconnection and deprecation.RTM v1 is deprecated; consider Slack SDK WebClient/Socket Mode. Current recovery only retries connect once on ConnectionResetError.
If staying on RTM short-term, add a simple reconnect loop with backoff and health logs.
|
@coderabbitai generate docstrings |
✅ Actions performedDocstrings generation started. |
Docstrings generation was requested by @joon321. * #1 (comment) The following files were modified: * `SlackBot.py`
|
Note Generated docstrings for this pull request at #2 |
1 participant
Summary by CodeRabbit
Documentation
Chores