Improve Codex session detection and Telegram turn notifications#8
Conversation
WalkthroughThis PR introduces a turn completion notification system with configurable user mention list, improves history preview formatting with deduplication and line normalization, refines git worktree and session metadata handling to treat worktrees as separate environments, updates markdown rendering for link handling and inline formatting rules, and applies minor code maintenance improvements for readability and optimization. ChangesTurn Completion Notification System
History Preview Improvements
Git Worktree and Session Metadata Handling
Markdown Rendering and Link Processing
Code Maintenance and Optimization
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/codex_history.rs (1)
953-961:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPreserve repo/worktree root identity for nested subdirectories.
This change makes
environment_identity_for_cwd()fall back to the exactcwdfor any path below the repo/worktree root, so a session started fromrepo/srcwill no longer group with one started fromrepo/. That’s a broader behavior change than the PR objective and can split a single Telegram environment into multiple topics.Suggested fix
fn git_environment_root(cwd: &Path) -> Option<PathBuf> { - let git_path = cwd.join(".git"); - if git_path.is_dir() { - return Some(normalize_path(cwd.to_path_buf())); - } - if git_path.is_file() { - return Some(normalize_path(cwd.to_path_buf())); - } + for ancestor in cwd.ancestors() { + let git_path = ancestor.join(".git"); + if git_path.is_dir() || git_path.is_file() { + return Some(normalize_path(ancestor.to_path_buf())); + } + } None }🤖 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/codex_history.rs` around lines 953 - 961, git_environment_root currently only checks the given cwd for a .git entry, causing environment_identity_for_cwd to treat nested subdirectories (e.g., repo/src) as distinct identities; change git_environment_root to walk up the ancestor chain from cwd to root, checking each ancestor for a .git file or directory and return normalize_path of the ancestor where .git is found (so sessions under the same repo/worktree share the same identity). Update the logic in git_environment_root (referenced by environment_identity_for_cwd) to loop through parents rather than only inspecting cwd.
🧹 Nitpick comments (2)
src/app/turns.rs (1)
328-333: ⚡ Quick winMake the completion notification text configurable (or at least a constant).
turn_completion_notification_texthardcodes a Russian-language string with an emoji ("Готово, … fyi ✅"). This bakes a locale assumption into a feature that's wired to a genericcompletion_notify_usernamesconfig knob, and as you mentioned in the PR description, exposing the template is the natural next step. A simple path is to add an optionaltelegram.completion_notify_template(orcompletion_notify_text) string with the current value as the default, and substitute{usernames}(or{mentions}) at format time. This also makes the function testable for non-Russian setups without code changes.Happy to follow up with a concrete patch in this PR or a separate one — your call.
🤖 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/app/turns.rs` around lines 328 - 333, turn_completion_notification_text currently returns a hardcoded Russian string; make it configurable by reading an optional config value (e.g., telegram.completion_notify_template or completion_notify_text) and falling back to the current "Готово, {usernames} fyi ✅" template; update turn_completion_notification_text to accept the template (or fetch it from the config) and format it by replacing a placeholder like {usernames} (or {mentions}) with usernames.join(" "), returning None when the list is empty, so tests and non-Russian setups can override the message without code changes.src/config.rs (1)
274-295: 💤 Low valueMinor:
trim_start_matches('@')repeats —strip_prefix('@')matches a single sigil.
trim_start_matches('@')will silently accept"@@user"or"@@@user"by stripping every leading@. Switching tostrip_prefix('@').unwrap_or(username)would normalize exactly one leading@, matching Telegram conventions and surfacing typos earlier via the ASCII allowlist if more@s sneak in. Optional polish.♻️ Proposed tweak
fn normalize_completion_notify_username(input: &str) -> Result<String> { - let username = input.trim().trim_start_matches('@'); + let trimmed = input.trim(); + let username = trimmed.strip_prefix('@').unwrap_or(trimmed);🤖 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/config.rs` around lines 274 - 295, The helper normalize_completion_notify_username currently uses trim_start_matches('@') which removes any number of leading '@' characters; change it to remove at most one sigil by replacing that call with username = input.trim().strip_prefix('@').unwrap_or(input.trim()); (or equivalent) so you only normalize a single leading '@', preserving detection of inputs like "@@user" by the ASCII allowlist/length checks in normalize_completion_notify_username and keeping the rest of the validation logic intact.
🤖 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.
Outside diff comments:
In `@src/codex_history.rs`:
- Around line 953-961: git_environment_root currently only checks the given cwd
for a .git entry, causing environment_identity_for_cwd to treat nested
subdirectories (e.g., repo/src) as distinct identities; change
git_environment_root to walk up the ancestor chain from cwd to root, checking
each ancestor for a .git file or directory and return normalize_path of the
ancestor where .git is found (so sessions under the same repo/worktree share the
same identity). Update the logic in git_environment_root (referenced by
environment_identity_for_cwd) to loop through parents rather than only
inspecting cwd.
---
Nitpick comments:
In `@src/app/turns.rs`:
- Around line 328-333: turn_completion_notification_text currently returns a
hardcoded Russian string; make it configurable by reading an optional config
value (e.g., telegram.completion_notify_template or completion_notify_text) and
falling back to the current "Готово, {usernames} fyi ✅" template; update
turn_completion_notification_text to accept the template (or fetch it from the
config) and format it by replacing a placeholder like {usernames} (or
{mentions}) with usernames.join(" "), returning None when the list is empty, so
tests and non-Russian setups can override the message without code changes.
In `@src/config.rs`:
- Around line 274-295: The helper normalize_completion_notify_username currently
uses trim_start_matches('@') which removes any number of leading '@' characters;
change it to remove at most one sigil by replacing that call with username =
input.trim().strip_prefix('@').unwrap_or(input.trim()); (or equivalent) so you
only normalize a single leading '@', preserving detection of inputs like
"@@user" by the ASCII allowlist/length checks in
normalize_completion_notify_username and keeping the rest of the validation
logic intact.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a7076f3d-889e-4ade-bec6-5945363cebdb
📒 Files selected for processing (8)
src/app/presentation.rssrc/app/tests.rssrc/app/turns.rssrc/codex.rssrc/codex_history.rssrc/config.rssrc/render.rssrc/telegram.rs
2 participants
Summary
This PR contains fixes and small UX improvements discovered while dogfooding Telecodex with multiple
Codex sessions mapped to Telegram forum topics.
Changes:
repository root.
session_meta, so forked/resumed sessionskeep the correct cwd.
[repo](/home/...)are rendered as text instead of invalid Telegram HTMLlinks;
│are normalized before blockquote rendering;telegram.completion_notify_usernames, which sends a separate completion FYI message aftersuccessful turns.
Context
Honest note: this PR is vibe-coded with Codex. I am primarily a Go developer and do not want to pretend
deep Rust expertise. The changes came from a real Telecodex setup with several parallel Codex sessions
in separate git worktrees and Telegram forum topics.
I tested the behavior locally and would appreciate careful maintainer review. I am also happy to split
this into smaller PRs or reshape the config/API if that fits the project better.
Testing
cargo testcargo clippy --all-targets --all-features -- -D warningscargo build --releaseNotes
No local deployment config or secrets are included in this branch.
The completion notification currently formats as
Готово, @user fyi ✅to match my local setup. Ifpreferred, I can make the message text fully configurable instead of only configuring the usernames.