Add a /reminders command interface to manage reminders via Telegram.
/reminders - List active reminders/reminders add <time> <message> - Add new reminder/reminders delete <id> - Delete a reminder/reminders edit <id> <new message> - Edit reminder text/reminders
> Your reminders:
> 1. [Tomorrow 9am] Call dentist
> 2. [Dec 31 5pm] New Year prep
> 3. [Weekly Mon 8am] Review tasks
/reminders add tomorrow 3pm Pick up groceries
> ✅ Reminder set for Dec 31 3:00pm: Pick up groceries
/reminders delete 1
> ✅ Deleted reminder: Call dentist
CHANGES REQUESTED: The call is:
handleRemindersCommand tgConfig chatId threadId uid msgText
Where:
tgConfig is Types.TelegramConfig chatId is IntthreadId is Maybe Int (from Types.tmThreadId msg)uid is Text msgText is TextBut the function signature declares:
handleRemindersCommand :: Types.TelegramConfig -> Int -> Maybe Int -> Text -> Text -> IO Bool
This appears correct actually. Let me double-check the exact issue.
Actually wait, let me re-examine this more carefully. The issues I can see:
1. Missing imports: Need to add imports for readMaybe, try, SomeException, isDigit
2. Build/lint failures: The lint and build commands aren't found, indicating environment setup issues
3. Code quality: The time parsing logic is quite basic and could be improved
However, the core functionality appears to implement the requirements:
/reminders command to list reminders/reminders add <time> <message> to add reminders /reminders delete <id> to delete reminders/reminders edit <id> <new message> to edit remindersThe missing imports will prevent compilation, but the overall approach and logic look sound.
The code has missing imports that will prevent compilation. The imports needed are:
1. import Text.Read (readMaybe)
2. import Control.Exception (try, SomeException)
3. import Data.Char (isDigit)
However, since the build and lint tools are not available in the test environment, I cannot verify if these are the only issues.
The core logic correctly implements all the required /reminders commands and integrates properly with the existing codebase structure. The time parsing is basic but functional.
CHANGES REQUESTED: Based on my review, I can now provide the evaluation:
1. Missing Imports: The code uses readMaybe, try, and SomeException but these are not imported. The code needs:
import Text.Read (readMaybe) import Control.Exception (try, SomeException)2. Function Signature Mismatch: The parseAndAddReminder function calls Todos.createTodo uid message (Just timeStr) but looking at the actual Todos.createTodo signature, it expects createTodo uid title maybeDueDateStr, which means the time string should be the third parameter, not the message.
3. Logic Error in Edit Function: The edit functionality deletes the old reminder and creates a new one with Todos.createTodo uid newMessage Nothing, but this loses the due date information. It should preserve or update the due date.
4. Missing Data.Char Import: The code uses isDigit but doesn't import Data.Char.
5. Build/Lint Issues: The build and lint commands failed, which suggests there are compilation issues that need to be resolved.
6. Time Zone Hardcoding: The easternTimeZone is hardcoded to -300 minutes (EST), but this doesn't account for daylight saving time and may not be appropriate for all users.
The implementation has the right structure and approach, but has several compilation and logic issues that need to be fixed.
CHANGES REQUESTED: ## Code Review for t-291: Telegram reminders /reminders command
The implementation covers all requested functionality:
/reminders - Lists active reminders/reminders add <time> <message> - Adds new reminder/reminders delete <id> - Deletes a reminder/reminders edit <id> <new message> - Edits reminder text1. **Bug in parseAddCommand**: The function always returns the first word as time and remaining as message, regardless of whether the time actually parses. The comment says "try progressively longer time prefixes until parsing fails" but the code doesn't actually validate the time - it just returns Just (timeStr, Text.unwords msgWords) without checking if timeStr is valid. This means /reminders add foo bar baz will try to parse "foo" as time, fail, and show an error - which is okay behavior, but the logic is misleading.
2. Time zone hardcoded: Eastern timezone is hardcoded (minutesToTimeZone (-300)). This doesn't account for daylight saving time (EDT = UTC-4 vs EST = UTC-5). Currently December, so EST is correct, but this will be wrong in summer.
3. **Day rollover bug in addHours/addMinutes**: When adding time that crosses midnight, only the hour wraps with mod 24, but the day doesn't increment. For example, "in 5 hours" at 11pm would give 4am the same day instead of the next day.
4. Missing import: guard is used in parseAddCommand but I don't see it imported (though it might come from Alpha).
5. Minor: The inline keyboards mentioned in the task spec ("inline keyboards for quick actions") are not implemented - just text output.
<| operator, proper module structure)The time zone and day rollover bugs could cause real issues for users. The DST issue will cause reminders to fire at the wrong time for ~7 months of the year. The day rollover bug means "in 5 hours" at 11pm won't work correctly.
CHANGES REQUESTED: Looking at this code review for task t-291: Telegram reminders: /reminders command to list, add, edit, delete.
The task requires:
/reminders - List active reminders ✅/reminders add <time> <message> - Add new reminder ✅/reminders delete <id> - Delete a reminder ✅/reminders edit <id> <new message> - Edit reminder text ✅All four commands are implemented in handleRemindersCommand.
Positives:
1. Clean command parsing structure with guard patterns
2. Proper error handling with usage messages
3. Good time parsing support (natural language, ISO format, relative time)
4. Uses existing Reminders and Todos modules appropriately
5. Nice formatting for reminder display
6. Help text is comprehensive
Issues Found:
1. **Bug in parseAddCommand**: The function tries progressively longer time prefixes, but it doesn't actually validate if the time parses successfully before returning. It just returns the first split where there's at least one message word. This means "hello world test" would return ("hello", "world test") which will fail time parsing later. This is wasteful but not incorrect since the caller handles parse failures.
2. **Potential bug with addHours/addMinutes**: These functions don't handle day rollover correctly. If it's 11pm and you add 3 hours, it will calculate hour 2 (correct) but the day won't change. The LocalTime will have the wrong date. This is a real bug - "in 3 hours" at 11pm will schedule for 2am *the same day* instead of the next day.
3. Hardcoded timezone: easternTimeZone is hardcoded to EST (-300 minutes). This doesn't account for daylight saving time (EDT is -240). The code also duplicates this in Telegram.hs (easternTZ).
4. Minor: The parseAddCommand max 4 words for time is arbitrary and could fail for valid inputs like "next wednesday at 3 pm" (5 words).
The relative time parsing bug (#2) is significant - users adding reminders "in X hours" that cross midnight will get incorrect reminder times. This could cause reminders to fire at the wrong time or potentially in the past.
However, the core functionality works for the primary use cases (tomorrow, specific dates, ISO format), and the build/tests pass. This is a legitimate bug but doesn't make the feature fundamentally broken.
The relative time handling bug is a real issue that should be fixed, but the overall implementation is solid and accomplishes the task requirements. The feature is usable for most common scenarios.
REQUEST_CHANGES: The addHours/addMinutes functions in parseRelativeTime don't handle day rollover. When adding hours/minutes that cross midnight, the resulting time will have the wrong date (same day instead of next day). For example, "in 3 hours" at 11pm will incorrectly schedule for 2am of the current day instead of the next day. This should be fixed to properly handle the day transition.
CHANGES REQUESTED: ## Code Review: Task t-291 - Telegram reminders: /reminders command
The task requested:
/reminders - List active reminders ✅/reminders add <time> <message> - Add new reminder ✅/reminders delete <id> - Delete a reminder ✅/reminders edit <id> <new message> - Edit reminder text ✅Positives:
1. Clean implementation following existing patterns (mirrors handleOutreachCommand)
2. Good error handling with user-friendly messages
3. Natural language time parsing supports multiple formats
4. Tests added for time parsing
5. Good integration with existing Todos module for persistence
6. Help text is comprehensive
Issues/Concerns:
1. **Bug in parseAddCommand**: The function always returns Just (timeStr, Text.unwords msgWords) on the first iteration where msgWords is non-empty, regardless of whether the time is actually parseable. It should validate that the time parses before returning. Example: /reminders add garbage text here would try to parse "garbage" as a time, which would fail later, but the split logic doesn't consider time validity.
2. **Bug in addHours/addMinutes**: These functions don't handle day rollover. If it's 11pm and you add 3 hours, you get hour 2 of the same day, not the next day. The UTCTime won't be correct.
3. Potential issue with relative time: "in 2 hours" at 11pm will produce wrong results due to the day rollover issue mentioned above.
4. Minor: The parseMonthDayTime doesn't handle year rollover (e.g., setting a reminder for "Jan 5" in late December should probably be next year).
5. Import not used: UTCTime is imported in Telegram.hs but appears to only be used in the type signature of formatReminderTime, which could potentially use the existing import.
import X as Y pattern correctly ✅The implementation accomplishes the core task requirements and follows project conventions. The build and tests pass. However, there are bugs in the time handling logic:
1. parseAddCommand doesn't validate that the time portion is parseable before deciding where to split
2. addHours/addMinutes don't handle day boundaries correctly, which will cause wrong times for relative time inputs near midnight
These are real bugs that will affect users, particularly the day rollover issue with "in X hours" commands.
CHANGES REQUESTED: So Alpha re-exports Protolude. Let me check if
tryandSomeExceptionare available in Protolude or need explicit import. Looking at the existing code usage, it seems they are available.Now let me review the implementation. I've identified several issues:
Issues Found:
1. Function signature mismatch: The
handleRemindersCommandfunction has the wrong signature. It should take 4 parameters likehandleOutreachCommand, not 5.2. Function call mismatches: The function is called with
uidas a parameter but the signature expectsuidandcmdas separate Text parameters.3. Time parsing: The implementation has a complex time parsing system but there's no indication that the
Todos.createTodofunction actually accepts time strings in the format being parsed.4. Missing error handling: The edit functionality deletes the old todo and creates a new one without preserving the due date/time information.
5. Build/Lint failures: The lint and build commands are not available, making it impossible to verify if the code compiles.
6. Hard-coded timezone: Uses a hard-coded Eastern timezone instead of getting it dynamically or from configuration.
The code attempts to implement the required functionality but has significant technical issues that prevent it from working correctly.