Add IRequestsParamsProvider to allow passing additional url params to…#81
Conversation
|
Warning Review limit reached
More reviews will be available in 12 minutes and 31 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, 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 include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughThis PR introduces a new global interface 🚥 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)
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 |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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.
Inline comments:
In `@framework/update/internal/appupdateservice.cpp`:
- Around line 138-147: The code currently creates an empty QUrlQuery which
overwrites any existing query parameters on the QUrl `url`; change the
initialization to start from the existing URL so pre-existing parameters are
preserved (e.g., construct QUrlQuery from `url` or its existing query), then add
the provider params from `requestParamsProvider()->requestParams()` and call
`url.setQuery(query)` as before; update the block around
`requestParamsProvider()`, the `QUrlQuery query` initialization, and the loop
that adds items to reference that preserved-initialized `query`.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 81b13a09-6cd8-466e-a702-22e9292de853
📒 Files selected for processing (4)
framework/global/CMakeLists.txtframework/global/irequestparamsprovider.hframework/update/internal/appupdateservice.cppframework/update/internal/appupdateservice.h
…ams to the update checker
38278e7 to
b454fb4
Compare
| } | ||
|
|
||
| QUrl url = QString::fromStdString(configuration()->checkForAppUpdateUrl()); | ||
| if (requestParamsProvider()) { |
There was a problem hiding this comment.
let's remove this if, this is an internal service and will always exist in this module
There was a problem hiding this comment.
The idea is that this provider is set by the app, and the framework only provides an interface. That is, only the app knows which params to add.
There was a problem hiding this comment.
Similar to IShortcutContextPriority and a few other services
There was a problem hiding this comment.
No, it's still worth registering interfaces in those modules rather than relying on someone else to register them somewhere
There was a problem hiding this comment.
Hm, I am confused. Please help me understand, what framework side params provider should do.
This is an excerp from pending auadicty PR:
std::vector<std::pair<std::string, std::string> > UsageInfoService::updateRequestParams() const
{
std::vector<std::pair<std::string, std::string> > params;
if (!getSendAnonymousUsageInfo()) {
return params;
}
params.emplace_back("audacity-instance-id", instanceId());
if (authorization()) {
const std::string userId = authorization()->accountInfo().id;
if (!userId.empty()) {
params.emplace_back("user_id", userId);
}
}
return params;
}| GlobalInject<io::IFileSystem> fileSystem; | ||
| GlobalInject<ISystemInfo> systemInfo; | ||
| GlobalInject<IUpdateConfiguration> configuration; | ||
| GlobalInject<IUpdateRequestParamsProvider> requestParamsProvider; |
There was a problem hiding this comment.
you didn't register this service in the module for use
Add IRequestsParamsProvider to allow passing additional url params to the update checker
Build configuration
audacity: audacity/audacity/master
audacity platforms: linux_x64
musescore: musescore/MuseScore/main
musescore platforms: linux_x64