Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Greptile Summary
This PR fixes a critical timezone handling bug in the Slack bot that was causing TypeError: can't subtract offset-naive and offset-aware datetimes when formatting document timestamps for display in Slack messages. The error occurred in the timeago.format() function when it tried to subtract a timezone-naive d.updated_at datetime from a timezone-aware datetime.now(pytz.utc) datetime.
The fix introduces a new helper function _format_doc_updated_at() that centralizes timezone handling logic. This function checks if a datetime is timezone-naive (no timezone info) and converts it to UTC if needed, or converts timezone-aware datetimes to UTC before passing them to timeago.format(). The helper function is now used in two places where the error was occurring: _build_documents_blocks() and _build_sources_blocks().
The solution follows the codebase's principles by:
- Creating a reusable function to avoid code duplication
- Ensuring consistent timezone handling across the application
- Following the "fail loudly" principle by properly handling timezone conversion rather than silently failing
- Using explicit type annotations and clear function documentation
Comprehensive unit tests were added to verify the fix works correctly with both timezone-naive and timezone-aware datetime objects, ensuring the bug won't reoccur.
Confidence score: 5/5
- This PR is safe to merge with minimal risk as it fixes a critical runtime error without changing core functionality
- Score reflects a straightforward bug fix with proper testing and defensive programming practices
- No files require special attention as the changes are well-contained and thoroughly tested
3 files reviewed, no comments
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
2 issues found across 3 files
Prompt for AI agents (all 2 issues)
Understand the root cause of the following 2 issues and fix them.
<file name="backend/tests/unit/onyx/onyxbot/test_slack_blocks.py">
<violation number="1" location="backend/tests/unit/onyx/onyxbot/test_slack_blocks.py:61">
Hard-coded relative time in test makes it time-dependent and flaky; compute expected text dynamically using timeago.format and current UTC time.</violation>
<violation number="2" location="backend/tests/unit/onyx/onyxbot/test_slack_blocks.py:62">
Brittle, time-dependent assertion: exact match on relative time string will fail as time passes. Make the assertion robust (e.g., prefix/suffix check) or freeze now.</violation>
</file>
React with 👍 or 👎 to teach cubic. Mention @cubic-dev-ai to give feedback, ask questions, or re-run the review.
| assert len(blocks) >= 2 | ||
| section_block = blocks[1].to_dict() | ||
| # NOTE: will need to be updated after another year. | ||
| expected_text = "<https://example.com|Example Doc>\n_Updated 1 year ago_\n>" |
There was a problem hiding this comment.
Hard-coded relative time in test makes it time-dependent and flaky; compute expected text dynamically using timeago.format and current UTC time.
Prompt for AI agents
Address the following comment on backend/tests/unit/onyx/onyxbot/test_slack_blocks.py at line 61:
<comment>Hard-coded relative time in test makes it time-dependent and flaky; compute expected text dynamically using timeago.format and current UTC time.</comment>
<file context>
@@ -0,0 +1,67 @@
+ assert len(blocks) >= 2
+ section_block = blocks[1].to_dict()
+ # NOTE: will need to be updated after another year.
+ expected_text = "<https://example.com|Example Doc>\n_Updated 1 year ago_\n>"
+ assert section_block["text"]["text"] == expected_text
+
</file context>
✅ Addressed in f836312
There was a problem hiding this comment.
I think I agree with Cubic, might be better if we make it dynamic instead of hard coding the time to 1 year?
There was a problem hiding this comment.
Thanks! Agreed—I'll update the test to compute the expected relative time dynamically rather than hard-coding “1 year.”
jessicasingh7
left a comment
jessicasingh7
left a comment
There was a problem hiding this comment.
LGTM! Includes some of the change I also had it in my Slack PR so makes sense!
| assert len(blocks) >= 2 | ||
| section_block = blocks[1].to_dict() | ||
| # NOTE: will need to be updated after another year. | ||
| expected_text = "<https://example.com|Example Doc>\n_Updated 1 year ago_\n>" |
There was a problem hiding this comment.
I think I agree with Cubic, might be better if we make it dynamic instead of hard coding the time to 1 year?
2 participants
Description
Fixes:
How Has This Been Tested?
test
Backporting (check the box to trigger backport action)
Note: You have to check that the action passes, otherwise resolve the conflicts manually and tag the patches.