[ECO-5577] Rename QueryOptions to HistoryParams
#380
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 4 minutes and 2 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. 📒 Files selected for processing (7)
WalkthroughRenamed the history query parameter type from Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Msgs as Messages / DefaultMessages
participant Sub as SubscriptionResponse
participant API as ChatAPI
participant Net as Network
rect rgb(235,245,255)
Note right of Msgs: Public history API
Msgs->>API: getMessages(roomName, params: HistoryParams)
API->>Net: HTTP GET /rooms/:roomName/messages?{params.asQueryItems()}
Net-->>API: HTTP response (paginated messages)
API-->>Msgs: PaginatedResult<Message>
end
rect rgb(255,245,235)
Note right of Sub: Pre-subscribe history
Sub->>API: getMessages(roomName, params: HistoryParams(with orderBy=newestFirst, fromSerial?))
API->>Net: HTTP GET /rooms/:roomName/messages?{...}
Net-->>API: HTTP response
API-->>Sub: PaginatedResult<Message>
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
Pre-merge checks and finishing touches and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
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 |
lawrence-forooghian
force-pushed
the
rename-QueryOptions-to-HistoryParams
branch
from
47748d7 to
4e4f985
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
🧹 Nitpick comments (6)
Tests/AblyChatTests/DefaultMessagesTests.swift (1)
439-439: Tests updated to new API — LGTMCalls now use history(withParams:). Consider renaming test names mentioning “QueryOptions” to “HistoryParams” for clarity.
Also applies to: 470-470
Sources/AblyChat/Subscription.swift (1)
57-57: Rename to withParams and HistoryParams — LGTMBehavior preserved: force newestFirst and set fromSerial before fetching.
Minor: update doc text “Options” → “Params” for consistency with the new naming.
Also applies to: 94-105
Sources/AblyChat/Messages.swift (4)
36-36: Public API now uses HistoryParams — LGTMMethod signature updated; aligns with downstream implementations and tests.
Docs above still refer to “options”; consider updating parameter docs to “params”.
233-233: Introduce HistoryParams — LGTMNaming aligns with API guidelines. Consider updating the struct comment from “Options” to “Parameters” for consistency.
285-316: Default direction handling: verify or make explicitDocs say default is newestFirst, but asQueryItems omits “direction” when orderBy is nil, relying on server defaults. To avoid ambiguity, either:
- Confirm server default is backwards/newestFirst, or
- Explicitly set “direction=backwards” when orderBy is nil.
Example change:
func asQueryItems() -> [String: String] { var dict: [String: String] = [:] @@ - if let orderBy { - switch orderBy { - case .oldestFirst: - dict["direction"] = "forwards" - case .newestFirst: - dict["direction"] = "backwards" - } - } + switch orderBy { + case .some(.oldestFirst): + dict["direction"] = "forwards" + case .some(.newestFirst): + dict["direction"] = "backwards" + case .none: + // Be explicit to match documented default + dict["direction"] = "backwards" + }Alternatively, if you intend to rely on server defaults, update the HistoryParams docs to state that omission defers to server defaults.
233-233: Optional: provide migration shims for smoother adoptionTo ease external migration, consider temporarily adding:
- public typealias QueryOptions = HistoryParams
- a deprecated wrapper on Messages: history(withOptions:) calling history(withParams:)
Example:
public typealias QueryOptions = HistoryParams public extension Messages { @available(*, deprecated, message: "Use history(withParams:) instead.") func history(withOptions options: QueryOptions) async throws(ARTErrorInfo) -> HistoryResult { try await history(withParams: options) } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (7)
Example/AblyChatExample/Mocks/MockClients.swift(1 hunks)Example/AblyChatExample/Mocks/MockSubscriptionStorage.swift(4 hunks)Sources/AblyChat/ChatAPI.swift(1 hunks)Sources/AblyChat/DefaultMessages.swift(1 hunks)Sources/AblyChat/Messages.swift(5 hunks)Sources/AblyChat/Subscription.swift(2 hunks)Tests/AblyChatTests/DefaultMessagesTests.swift(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (6)
Sources/AblyChat/DefaultMessages.swift (2)
Example/AblyChatExample/Mocks/MockClients.swift (1)
history(157-159)Sources/AblyChat/ChatAPI.swift (1)
getMessages(19-22)
Tests/AblyChatTests/DefaultMessagesTests.swift (2)
Example/AblyChatExample/Mocks/MockClients.swift (1)
history(157-159)Sources/AblyChat/DefaultMessages.swift (1)
history(118-124)
Sources/AblyChat/Messages.swift (3)
Example/AblyChatExample/Mocks/MockClients.swift (1)
history(157-159)Sources/AblyChat/DefaultMessages.swift (1)
history(118-124)Tests/AblyChatTests/MessageSubscriptionAsyncSequenceTests.swift (1)
mockGetPreviousMessages(49-57)
Sources/AblyChat/Subscription.swift (1)
Example/AblyChatExample/Mocks/MockSubscriptionStorage.swift (1)
historyBeforeSubscribe(211-213)
Example/AblyChatExample/Mocks/MockClients.swift (1)
Sources/AblyChat/DefaultMessages.swift (1)
history(118-124)
Example/AblyChatExample/Mocks/MockSubscriptionStorage.swift (1)
Sources/AblyChat/Subscription.swift (1)
historyBeforeSubscribe(94-109)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (12)
- GitHub Check: SPM,
releaseconfiguration (Xcode 26.0) - GitHub Check: Xcode, macOS (Xcode 26.0)
- GitHub Check: Xcode, tvOS (Xcode 26.0)
- GitHub Check: Xcode, iOS (Xcode 26.0)
- GitHub Check: SPM (Xcode 26.0)
- GitHub Check: Xcode,
releaseconfiguration, iOS (Xcode 26.0) - GitHub Check: Xcode,
releaseconfiguration, macOS (Xcode 26.0) - GitHub Check: Xcode,
releaseconfiguration, tvOS (Xcode 26.0) - GitHub Check: Example app, macOS (Xcode 26.0)
- GitHub Check: Example app, tvOS (Xcode 26.0)
- GitHub Check: Example app, iOS (Xcode 26.0)
- GitHub Check: spec-coverage
🔇 Additional comments (6)
Example/AblyChatExample/Mocks/MockSubscriptionStorage.swift (1)
130-130: Consistent rename to HistoryParams — LGTMClosures, method, and initializer now uniformly use HistoryParams. No behavioral changes introduced.
Also applies to: 155-155, 210-210, 211-211, 221-221
Sources/AblyChat/DefaultMessages.swift (1)
118-118: Method rename wired correctly — LGTMhistory(withParams:) forwards params to chatAPI.getMessages as expected.
Also applies to: 120-120
Sources/AblyChat/ChatAPI.swift (1)
19-19: getMessages now accepts HistoryParams — LGTMSignature aligns with callers; asQueryItems() remains the conversion point.
Example/AblyChatExample/Mocks/MockClients.swift (1)
157-157: Mock API updated to HistoryParams — LGTMSignature change only; behavior unchanged.
Sources/AblyChat/Messages.swift (2)
277-277: Initializer updated to HistoryParams.OrderBy — LGTMType reference updated correctly.
367-367: AsyncSequence hooks migrated to HistoryParams — LGTMClosures and method now take HistoryParams; matches Subscription and Messages surfaces.
Also applies to: 372-372, 379-379, 394-394
lawrence-forooghian
force-pushed
the
rename-QueryOptions-to-HistoryParams
branch
from
a748402 to
4e4f985
Compare
|
Actually, no, will reset this PR and re-open #381. |
As in [1]. And accordingly rename Messages.history(withOptions:) to history(withParams:). [1] ably/ably-chat-js#657
lawrence-forooghian
force-pushed
the
rename-QueryOptions-to-HistoryParams
branch
from
4e4f985 to
3aedfc2
Compare
lawrence-forooghian
changed the title
QueryOptions to HistoryParamsQueryOptions to HistoryParams
As in ably/ably-chat-js#657. And accordingly rename
Messages.history(withOptions:)tohistory(withParams:).Summary by CodeRabbit
Refactor
Tests