Fix decoding of Message.createdAt
#99
Conversation
|
Warning Rate limit exceeded@lawrence-forooghian has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 13 minutes and 52 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. WalkthroughThe changes made to the Changes
Possibly related PRs
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
lawrence-forooghian
force-pushed
the
fix-Message-createdAt-decoding
branch
from
142ebf5
to
77456aa
Compare
![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
🧹 Outside diff range and nitpick comments (2)
Tests/AblyChatTests/Mocks/MockHTTPPaginatedResponse.swift (1)
Line range hint
108-114: Consider addingcreatedAtto all mock messages.For comprehensive testing of the
Message.createdAtdecoding functionality, consider adding thecreatedAtfield to other mock messages in both the main response and the next page. This ensures consistent testing of the date decoding across all mock data.Apply this diff to add timestamps to other messages:
[ "clientId": "random", "timeserial": "3446457", + "createdAt": 1_730_943_049_270, "roomId": "basketball::$chat::$chatMessages", "text": "hello response", "metadata": [:], "headers": [:], ], // In nextPage: [ "timeserial": "3446450", + "createdAt": 1_730_943_049_268, "roomId": "basketball::$chat::$chatMessages", "text": "previous message", "metadata": [:], "headers": [:], ], [ "timeserial": "3446451", + "createdAt": 1_730_943_049_267, "roomId": "basketball::$chat::$chatMessages", "text": "previous response", "metadata": [:], "headers": [:], ],Also applies to: 146-154
Sources/AblyChat/ChatAPI.swift (1)
Line range hint
67-76: Consider encapsulating the timestamp conversion logic.While the conversion from milliseconds to seconds is correct, it would be better to encapsulate this logic to ensure consistency across the codebase and improve maintainability.
Consider creating a dedicated extension:
+ private extension Int64 { + var millisecondsToDate: Date { + Date(timeIntervalSince1970: TimeInterval(Double(self) / 1000)) + } + } let message = Message( timeserial: response.timeserial, clientID: clientId, roomID: roomId, text: params.text, - createdAt: Date(timeIntervalSince1970: createdAtInSeconds), + createdAt: response.createdAt.millisecondsToDate, metadata: params.metadata ?? [:], headers: params.headers ?? [:] )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
Sources/AblyChat/ChatAPI.swift(1 hunks)Tests/AblyChatTests/ChatAPITests.swift(1 hunks)Tests/AblyChatTests/Mocks/MockHTTPPaginatedResponse.swift(1 hunks)
🔇 Additional comments (4)
Tests/AblyChatTests/Mocks/MockHTTPPaginatedResponse.swift (1)
102-102: LGTM: Timestamp format aligns with REST API requirements.
The updated timestamp format (milliseconds since epoch) is correct for testing the Message.createdAt decoding functionality.
Sources/AblyChat/ChatAPI.swift (1)
130-137: LGTM! Correct implementation of timestamp decoding.
The change properly configures the decoder to handle Ably's REST API date format (milliseconds since Unix epoch). The implementation is well-documented and follows best practices.
Let's verify that all date fields in the responses are properly handled:
✅ Verification successful
Let me check the Message model to verify the date field implementation.
Timestamp decoding implementation is correctly configured for all date fields
The verification confirms that:
Message.createdAtis the only date field in the models that needs decoding- The JSONDecoder with
.millisecondsSince1970strategy is consistently used across all decoding paths:- Direct JSON decoding via
decoderproperty - Dictionary decoding via
DictionaryDecoderwhich uses the same decoder instance
- Direct JSON decoding via
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other date fields in the codebase that might need similar treatment
rg -l "Date" | xargs rg "decode|Codable"
Length of output: 1594
Script:
#!/bin/bash
# Check Message model implementation
cat Sources/AblyChat/Message.swift
Length of output: 2681
Tests/AblyChatTests/ChatAPITests.swift (2)
Line range hint 171-185: LGTM! Good test coverage of timestamp scenarios.
The test effectively covers both cases:
- Message with a specific timestamp
- Message with
niltimestamp
This ensures the decoder can handle both scenarios correctly.
173-173: Verify the timestamp value and timezone handling.
The specific timestamp 1_730_943_049.269 (approximately March 6, 2024) seems to be chosen deliberately. However, we should verify that this matches the format expected by Ably's REST API.
✅ Verification successful
Let me gather more information about timestamp handling in the codebase.
Based on the search results, I can now provide a conclusive response about the timestamp verification:
The timestamp format is correctly aligned with Ably's REST API requirements
The code shows consistent timestamp handling where:
- The API receives timestamps in milliseconds (e.g.,
1_730_943_049_269in MockHTTPPaginatedResponse) - The
ChatAPIcorrectly converts milliseconds to seconds forDateinitialization - Test cases cover both scenarios: with timestamp (
1_730_943_049.269seconds) and without (nil)
The timestamp value in the test is consistent with the mock response data and properly represents the milliseconds-to-seconds conversion required by the API implementation.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if similar timestamp formats exist in other test files
# This helps verify consistency in timestamp handling across tests
# Search for timestamp patterns in test files
rg -g '**/*Tests.swift' '\d+_\d+\.\d+'
Length of output: 159
Script:
#!/bin/bash
# Check for timestamp-related code in the Message model and decoders
rg -g '*.swift' 'createdAt|timeIntervalSince1970' -A 3 -B 3
# Look for any date formatting or decoding logic
ast-grep --pattern 'class $_ : Codable {
$$$
createdAt: $_
$$$
}'
Length of output: 13737
lawrence-forooghian
force-pushed
the
fix-Message-createdAt-decoding
branch
from
77456aa
to
99136ef
Compare
lawrence-forooghian
force-pushed
the
fix-CI
branch
from
07f22bd
to
fee9204
Compare
Noticed this when writing an integration test for getPreviousMessages.
lawrence-forooghian
force-pushed
the
fix-Message-createdAt-decoding
branch
from
99136ef
to
9454e40
Compare
Note: This PR is based on top of #101; please review that one first.
Noticed this when writing an integration test for
getPreviousMessages.Summary by CodeRabbit
New Features
Bug Fixes
createdAttimestamp for messages and ensure robust error handling scenarios.Tests