m - #139
Merged
Merged
Conversation
Mod that colours HitObjects based on the musical division they are on, now in osu!catch https://github.com/user-attachments/assets/2dda493d-dc8b-4ea4-ba47-7d04e2062b42 For now *bananas are not coloured by the mod and keep their yellow colour*, since I think its better for the sake of readabilty (also it just looks kinda ugly?). Do leave your thoughts on that. Droplets are always coloured to `LightGreen`, setting their colour to closest timeline ticks is wrong and looks incorrect since droplets aren't generated with them in mind. --------- Co-authored-by: Bartłomiej Dach <dach.bartlomiej@gmail.com> Co-authored-by: Shavix <54279284+Shavixinio@users.noreply.github.com>
## [Adjust CI test reporting to upstream action changes](f736337) It's been semi-broken since I bumped it a few weeks ago. Paper trail for this is at dorny/test-reporter#750, but to TL;DR it: `dorny/test-reporter@>=v2.0.0` migrated from [creating new check runs via the GitHub API](https://docs.github.com/en/rest/checks/runs) to [job summaries](https://docs.github.com/en/actions/reference/workflows-and-actions/workflow-commands#adding-a-job-summary) by default. The main difference is that compared to check runs, job summaries do not *appear to* require extra `GITHUB_TOKEN` permissions, but in exchange are limited to the job that called it. This broke visibility of the test reports because due to `GITHUB_TOKEN` permissions foibles the test reporter was running in a separate workflow. I see migrating as a plus here, since: - The visibility of results is comparable-to-better (example available for preview at https://github.com/bdach/osu/actions/runs/23892493152) - No longer required to have a completely separate workflow for test result reporting - No longer required to give `checks: write` permissions to the action (I'd hope, we'll see, untested on a public repository with PRs involved) One downside is there'll be no in-code annotations for failing tests anymore but that's whatever IMO. Half the time they weren't even very helpful, test results pretty much require maintainer interpretation anyway. This needs to be applied to a few other repos but I'm starting here because this is the one where the traffic is highest and therefore unbreaking the report is of most value (and also the one where I'll see if it works with public PRs the fastest). Side note, I was hoping to remove the artifact upload/download games by just attaching the summary inside each individual test job in the matrix, but [it looks like crap](https://github.com/bdach/osu-framework/actions/runs/23888384309) because only the first three summaries are loaded by default, so if there are more, you have to click each remaining one to see its output. Wow. Awesome. Also updates the action to `v3.0.0` to resolve node deprecation warnings. ## [Update inspectcode version to resolve deprecation warnings](496cf68) More node deprecation warning fixes.
|
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughConsolidates NUnit test reporting into the main CI workflow, bumps an inspect-code action, adds a Catch synesthesia mod, and makes multiplayer referee-aware across client API, room permissions, UI/control flow, and many visual tests. Changes
Sequence Diagram(s)sequenceDiagram
participant UI as UI
participant Client as MultiplayerClient
participant Server as Server
participant Room as MultiplayerRoom
UI->>Client: Click Ready / Start / Abort / Stop Countdown
Client->>Room: Evaluate Room.State, ActiveCountdowns, IsHost, IsReferee, player counts
alt Start allowed (Host or Referee & conditions)
Client->>Server: Send StartMatch or StartMatchCountdownRequest
Server-->>Room: Update Room.State / ActiveCountdowns
Room-->>Client: RoomUpdated
Client-->>UI: Refresh buttons/visibility/colours
else Abort/Stop allowed (Host or Referee & conditions)
Client->>Server: Send AbortMatch or StopCountdownRequest
Server-->>Room: Update Room.State
Room-->>Client: RoomUpdated
Client-->>UI: Refresh buttons/visibility/colours
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
osu.Game.Rulesets.Catch/Mods/CatchModSynesthesia.cs (1)
37-49: Reset colour state on each apply and simplify branch order.At Line 37,
timingBasedColourcan carry stale state acrossHitObjectAppliedinvocations. Reset it at handler start, and makeDropletan early/first branch to avoid an unnecessary divisor lookup before overwriting.♻️ Proposed refactor
d.HitObjectApplied += _ => { - // Block bananas from getting coloured. - if (d.HitObject is not Banana) - { - timingBasedColour = BindableBeatDivisor.GetColourFor(currentBeatmap.ControlPointInfo.GetClosestBeatDivisor(d.HitObject.StartTime), colours); - } - - // Colour droplets into a solid colour, as droplets aren't generated snapped to timeline ticks. - if (d.HitObject is Droplet) - { - timingBasedColour = Color4.LightGreen; - } + timingBasedColour = null; + + // Colour droplets into a solid colour, as droplets aren't generated snapped to timeline ticks. + if (d.HitObject is Droplet) + { + timingBasedColour = Color4.LightGreen; + } + // Block bananas from getting coloured. + else if (d.HitObject is not Banana) + { + timingBasedColour = BindableBeatDivisor.GetColourFor(currentBeatmap.ControlPointInfo.GetClosestBeatDivisor(d.HitObject.StartTime), colours); + } };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@osu.Game.Rulesets.Catch/Mods/CatchModSynesthesia.cs` around lines 37 - 49, Reset timingBasedColour at the start of the HitObjectApplied handler and reorder the branches so Droplet is checked first to avoid doing a GetClosestBeatDivisor lookup that will be overwritten; specifically, in the HitObjectApplied lambda for the class containing timingBasedColour, set timingBasedColour to a default (e.g., null/transparent) at the top, then if (d.HitObject is Droplet) set timingBasedColour = Color4.LightGreen and return/skip further checks, else if (d.HitObject is not Banana) compute timingBasedColour via BindableBeatDivisor.GetColourFor(currentBeatmap.ControlPointInfo.GetClosestBeatDivisor(d.HitObject.StartTime), colours).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/ci.yml:
- Around line 112-138: The test-reporter action in the "test-results" job (uses:
dorny/test-reporter@v3.0.0) needs the GitHub Checks API write permission; update
the workflow permissions to include checks: write either at the top-level
workflow permissions block or as job-level permissions for the "test-results"
job so the action can create check runs and publish results. Ensure the
permissions block includes "checks: write" alongside existing permissions and
remove any conflicting scopes.
- Around line 52-53: The workflow step using JetBrains/ReSharper-InspectCode
references a non-existent tag v0.12; update the step that currently shows uses:
JetBrains/ReSharper-InspectCode@v0.12 to use the valid tag
JetBrains/ReSharper-InspectCode@v0.11 so the CI action resolves correctly.
---
Nitpick comments:
In `@osu.Game.Rulesets.Catch/Mods/CatchModSynesthesia.cs`:
- Around line 37-49: Reset timingBasedColour at the start of the
HitObjectApplied handler and reorder the branches so Droplet is checked first to
avoid doing a GetClosestBeatDivisor lookup that will be overwritten;
specifically, in the HitObjectApplied lambda for the class containing
timingBasedColour, set timingBasedColour to a default (e.g., null/transparent)
at the top, then if (d.HitObject is Droplet) set timingBasedColour =
Color4.LightGreen and return/skip further checks, else if (d.HitObject is not
Banana) compute timingBasedColour via
BindableBeatDivisor.GetColourFor(currentBeatmap.ControlPointInfo.GetClosestBeatDivisor(d.HitObject.StartTime),
colours).
🪄 Autofix (Beta)
❌ Autofix failed (check again to retry)
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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4191a571-7e2d-47a6-bdfe-72af01d72f1c
📒 Files selected for processing (4)
.github/workflows/ci.yml.github/workflows/report-nunit.ymlosu.Game.Rulesets.Catch/CatchRuleset.csosu.Game.Rulesets.Catch/Mods/CatchModSynesthesia.cs
💤 Files with no reviewable changes (1)
- .github/workflows/report-nunit.yml
Comment on lines
52
to
+53
| - name: InspectCode | ||
| uses: JetBrains/ReSharper-InspectCode@v0.11 | ||
| uses: JetBrains/ReSharper-InspectCode@v0.12 |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if the v0.12 tag exists for JetBrains/ReSharper-InspectCode
gh api repos/JetBrains/ReSharper-InspectCode/git/refs/tags/v0.12 --jq '.ref' 2>/dev/null || echo "Tag v0.12 not found"Repository: winnerspiros/osu
Length of output: 191
🌐 Web query:
JetBrains ReSharper InspectCode GitHub Action v0.12 release
💡 Result:
No evidence found of a JetBrains ReSharper InspectCode GitHub Action v0.12 release. The official GitHub repository https://github.com/JetBrains/ReSharper-InspectCode shows the latest release as v0.11 on 2025-10-09. Usage examples and Marketplace listings also reference v0.11 as the current version. As of 2026-04-02, v0.12 has not been released.
Citations:
- 1: https://github.com/JetBrains/ReSharper-InspectCode/releases
- 2: https://github.com/JetBrains/ReSharper-InspectCode
- 3: https://github.com/marketplace/actions/jetbrains-resharper-inspect-code
The action version v0.12 does not exist; use v0.11 instead.
JetBrains/ReSharper-InspectCode@v0.12 is not available. The latest version is v0.11 (released 2025-10-09). Update to uses: JetBrains/ReSharper-InspectCode@v0.11 to fix the workflow failure.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/ci.yml around lines 52 - 53, The workflow step using
JetBrains/ReSharper-InspectCode references a non-existent tag v0.12; update the
step that currently shows uses: JetBrains/ReSharper-InspectCode@v0.12 to use the
valid tag JetBrains/ReSharper-InspectCode@v0.11 so the CI action resolves
correctly.
Comment on lines
+112
to
+138
| test-results: | ||
| name: Test results | ||
| runs-on: ubuntu-latest | ||
| # we want to wait for the `test` job to complete, but run regardless of whether it succeeds or fails | ||
| # https://docs.github.com/en/actions/reference/workflows-and-actions/workflow-syntax#example-not-requiring-successful-dependent-jobs | ||
| if: ${{ !cancelled() }} | ||
| needs: test | ||
| timeout-minutes: 5 | ||
| steps: | ||
| - name: Checkout | ||
| uses: actions/checkout@v6 | ||
|
|
||
| - name: Download results | ||
| uses: actions/download-artifact@v8 | ||
| with: | ||
| pattern: osu-test-results-* | ||
| merge-multiple: true | ||
|
|
||
| - name: Add test results summary to workflow run | ||
| uses: dorny/test-reporter@v3.0.0 | ||
| with: | ||
| name: Results | ||
| path: "*.trx" | ||
| reporter: dotnet-trx | ||
| list-suites: 'failed' | ||
| list-tests: 'failed' | ||
| use-actions-summary: 'true' |
There was a problem hiding this comment.
Missing checks: write permission for dorny/test-reporter.
The dorny/test-reporter action requires checks: write permission to create check runs and publish test results. The current workflow-level permissions (lines 7-9) only include contents: read and security-events: write. Without this permission, the action will fail or be unable to post results.
🔧 Proposed fix to add the required permission
permissions:
contents: read # to fetch code (actions/checkout)
security-events: write # for reporting InspectCode issues
+ checks: write # for reporting test results (dorny/test-reporter)Alternatively, add job-level permissions if you prefer to scope it to only the test-results job:
test-results:
name: Test results
runs-on: ubuntu-latest
+ permissions:
+ checks: write
# we want to wait for the `test` job to complete, but run regardless of whether it succeeds or fails📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| test-results: | |
| name: Test results | |
| runs-on: ubuntu-latest | |
| # we want to wait for the `test` job to complete, but run regardless of whether it succeeds or fails | |
| # https://docs.github.com/en/actions/reference/workflows-and-actions/workflow-syntax#example-not-requiring-successful-dependent-jobs | |
| if: ${{ !cancelled() }} | |
| needs: test | |
| timeout-minutes: 5 | |
| steps: | |
| - name: Checkout | |
| uses: actions/checkout@v6 | |
| - name: Download results | |
| uses: actions/download-artifact@v8 | |
| with: | |
| pattern: osu-test-results-* | |
| merge-multiple: true | |
| - name: Add test results summary to workflow run | |
| uses: dorny/test-reporter@v3.0.0 | |
| with: | |
| name: Results | |
| path: "*.trx" | |
| reporter: dotnet-trx | |
| list-suites: 'failed' | |
| list-tests: 'failed' | |
| use-actions-summary: 'true' | |
| test-results: | |
| name: Test results | |
| runs-on: ubuntu-latest | |
| permissions: | |
| checks: write | |
| # we want to wait for the `test` job to complete, but run regardless of whether it succeeds or fails | |
| # https://docs.github.com/en/actions/reference/workflows-and-actions/workflow-syntax#example-not-requiring-successful-dependent-jobs | |
| if: ${{ !cancelled() }} | |
| needs: test | |
| timeout-minutes: 5 | |
| steps: | |
| - name: Checkout | |
| uses: actions/checkout@v6 | |
| - name: Download results | |
| uses: actions/download-artifact@v8 | |
| with: | |
| pattern: osu-test-results-* | |
| merge-multiple: true | |
| - name: Add test results summary to workflow run | |
| uses: dorny/test-reporter@v3.0.0 | |
| with: | |
| name: Results | |
| path: "*.trx" | |
| reporter: dotnet-trx | |
| list-suites: 'failed' | |
| list-tests: 'failed' | |
| use-actions-summary: 'true' |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/ci.yml around lines 112 - 138, The test-reporter action in
the "test-results" job (uses: dorny/test-reporter@v3.0.0) needs the GitHub
Checks API write permission; update the workflow permissions to include checks:
write either at the top-level workflow permissions block or as job-level
permissions for the "test-results" job so the action can create check runs and
publish results. Ensure the permissions block includes "checks: write" alongside
existing permissions and remove any conflicting scopes.
|
Note Autofix is a beta feature. Expect some limitations and changes as we gather feedback and continue to improve it. An unexpected error occurred while generating fixes: Resource not accessible by integration - https://docs.github.com/rest/git/trees#create-a-tree |
There was a problem hiding this comment.
Pull request overview
Adds the Synesthesia fun mod to the catch ruleset and updates CI to publish test results summaries directly from the main workflow.
Changes:
- Introduce
CatchModSynesthesia, colouring catch hit objects based on closest beat divisor (with special-casing for bananas/droplets). - Register the new mod in
CatchRulesetfun mods. - Simplify CI test reporting by removing the separate
workflow_runreporter workflow and adding atest-resultsjob inci.yml(also bumps InspectCode action version).
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
osu.Game.Rulesets.Catch/Mods/CatchModSynesthesia.cs |
Implements Synesthesia colouring for catch drawable hit objects. |
osu.Game.Rulesets.Catch/CatchRuleset.cs |
Exposes the new Synesthesia mod in the catch fun mod list. |
.github/workflows/report-nunit.yml |
Removes the old cross-workflow NUnit reporting job. |
.github/workflows/ci.yml |
Adds an in-workflow test-results summary job; updates action versions/conditions. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+52
to
+57
| // Need to set this every update to ensure it doesn't get overwritten by DrawableHitObject.OnApply() -> UpdateComboColour(). | ||
| d.OnUpdate += _ => | ||
| { | ||
| if (timingBasedColour != null) | ||
| d.AccentColour.Value = timingBasedColour.Value; | ||
| }; |
There was a problem hiding this comment.
d.OnUpdate is used to reapply AccentColour every frame for every drawable hit object. This can add per-frame overhead on maps with many objects and will also spam bindable change notifications. Consider reapplying only when needed (e.g. schedule a one-time apply after DrawableHitObject.Apply() finishes, and/or listen to d.AccentColour.BindValueChanged and restore the synesthesia colour only when it gets changed by UpdateComboColour()/skinning).
Comment on lines
155
to
159
| new CatchModMuted(), | ||
| new CatchModNoScope(), | ||
| new CatchModMovingFast(), | ||
| new CatchModSynesthesia(), | ||
| }; |
There was a problem hiding this comment.
A new fun mod is being added to the catch mod list, but there doesn't appear to be a corresponding mod test scene (there are existing ones under osu.Game.Rulesets.Catch.Tests/Mods/ for other catch mods). Adding a basic ModTestScene for CatchModSynesthesia would help prevent regressions (at minimum verifying fruits get recoloured and bananas are excluded).
|
Note Autofix is a beta feature. Expect some limitations and changes as we gather feedback and continue to improve it. An unexpected error occurred while generating fixes: Resource not accessible by integration - https://docs.github.com/rest/git/trees#create-a-tree |
…me (#37152) - Last part of / closes ppy/osu-server-spectator#406. - Remaining work on slots will be tracked in ppy/osu-server-spectator#405. This PR is a corollary of ppy/osu-server-spectator#453 and all of the dispensations referee users in a multiplayer have received therein. The goal here is to allow access to all relevant room management functions even if the referee in question isn't host, as well as to disallow access to all non-relevant functions to do with the actual match gameplay. I'm not going to lie, this logic *is* ugly. I would argue that it already *was* ugly on `master` and my goal was to operate with as light a touch as possible myself. But you could see this as copping out and that I should try to refactor some of this. I will try - but only after someone else's seen the initial approach and deemed it unsuitable. The logic in `MatchStartControl` is awful - there are so many moving pieces of state that dictate what can happen when with all the buttons, and yes, I am making it worse here. This time there is some test coverage. Not everything is covered, but the coverage should be on par in all components and pieces of relevant logic I touched that already had tests covering them. On that note, please forgive the diffstat size, but the tests *are* most of that size. --------- Co-authored-by: Dean Herbert <pe@ppy.sh>
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@osu.Game.Tests/Visual/Multiplayer/TestSceneMultiplayerMatchSubScreen.cs`:
- Line 377: The assertion message in TestSceneMultiplayerMatchSubScreen is
wrong: in the AddAssert call that targets
MultiplayerRoomPanel().Single().ChangeSettingsButton.Alpha and asserts
Is.GreaterThan(0), update the assertion description from "button hidden" to
reflect the expected visible state (e.g., "button visible") so the message
matches the check in the test method.
In `@osu.Game/Screens/OnlinePlay/Multiplayer/Participants/ParticipantPanel.cs`:
- Line 298: The UI currently shows host-only actions to referees (kickButton
visibility line uses (client.IsHost || client.IsReferee)), but the server RPCs
(IMultiplayerRoomServer.TransferHost, KickUser) and OnlineMultiplayerClient
forwarders will throw NotHostException for non-host callers; update the
permission checks so referees cannot invoke these RPCs: change the
visibility/enable condition on kickButton and any "give host" control to require
client.IsHost (remove client.IsReferee), and additionally add defensive checks
in the event handlers that call OnlineMultiplayerClient.TransferHost/KickUser to
verify client.IsHost before calling (or catch NotHostException and surface a
user-friendly error) so referees never trigger host-only RPCs.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7da6f308-59ca-4093-a0d6-1c75e73c1e8f
📒 Files selected for processing (13)
osu.Game.Tests/Visual/Multiplayer/TestSceneMatchStartControl.csosu.Game.Tests/Visual/Multiplayer/TestSceneMultiplayerMatchSubScreen.csosu.Game.Tests/Visual/Multiplayer/TestSceneMultiplayerParticipantsList.csosu.Game.Tests/Visual/Multiplayer/TestSceneMultiplayerQueueList.csosu.Game/Online/Multiplayer/MultiplayerClient.csosu.Game/Online/Multiplayer/MultiplayerRoom.csosu.Game/Screens/OnlinePlay/Multiplayer/Match/MatchStartControl.csosu.Game/Screens/OnlinePlay/Multiplayer/Match/MultiplayerCountdownButton.csosu.Game/Screens/OnlinePlay/Multiplayer/Match/MultiplayerReadyButton.csosu.Game/Screens/OnlinePlay/Multiplayer/Match/Playlist/MultiplayerQueueList.csosu.Game/Screens/OnlinePlay/Multiplayer/MultiplayerMatchSubScreen.csosu.Game/Screens/OnlinePlay/Multiplayer/MultiplayerRoomPanel.csosu.Game/Screens/OnlinePlay/Multiplayer/Participants/ParticipantPanel.cs
| AddUntilStep("button visible", () => this.ChildrenOfType<MultiplayerRoomPanel>().Single().ChangeSettingsButton.Alpha, () => Is.GreaterThan(0)); | ||
| AddStep("join other user", void () => MultiplayerClient.AddUser(new APIUser { Id = PLAYER_1_ID })); | ||
| AddStep("make other user host", () => MultiplayerClient.TransferHost(PLAYER_1_ID)); | ||
| AddAssert("button hidden", () => this.ChildrenOfType<MultiplayerRoomPanel>().Single().ChangeSettingsButton.Alpha, () => Is.GreaterThan(0)); |
There was a problem hiding this comment.
Assertion message is inconsistent with the expected behavior.
The assertion description says "button hidden" but the test expects Alpha > 0 (i.e., the button is visible). This appears to be a copy-paste error from TestChangeSettingsButtonVisibleForHost where the assertion checks Alpha == 0.
Proposed fix
- AddAssert("button hidden", () => this.ChildrenOfType<MultiplayerRoomPanel>().Single().ChangeSettingsButton.Alpha, () => Is.GreaterThan(0));
+ AddAssert("button still visible", () => this.ChildrenOfType<MultiplayerRoomPanel>().Single().ChangeSettingsButton.Alpha, () => Is.GreaterThan(0));📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| AddAssert("button hidden", () => this.ChildrenOfType<MultiplayerRoomPanel>().Single().ChangeSettingsButton.Alpha, () => Is.GreaterThan(0)); | |
| AddAssert("button still visible", () => this.ChildrenOfType<MultiplayerRoomPanel>().Single().ChangeSettingsButton.Alpha, () => Is.GreaterThan(0)); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@osu.Game.Tests/Visual/Multiplayer/TestSceneMultiplayerMatchSubScreen.cs` at
line 377, The assertion message in TestSceneMultiplayerMatchSubScreen is wrong:
in the AddAssert call that targets
MultiplayerRoomPanel().Single().ChangeSettingsButton.Alpha and asserts
Is.GreaterThan(0), update the assertion description from "button hidden" to
reflect the expected visible state (e.g., "button visible") so the message
matches the check in the test method.
| } | ||
|
|
||
| kickButton.Alpha = client.IsHost && !user.Equals(client.LocalUser) ? 1 : 0; | ||
| kickButton.Alpha = (client.IsHost || client.IsReferee) && !user.Equals(client.LocalUser) ? 1 : 0; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Host-management RPC definitions and call sites"
rg -n -C3 '\b(TransferHost|KickUser)\s*\(' --type=cs
echo
echo "Current host/referee permission checks"
rg -n -C3 'NotHostException|MultiplayerRoomUserRole\.Referee|IsReferee' --type=csRepository: winnerspiros/osu
Length of output: 50372
Referees still appear to call host-only room-management RPCs.
This exposes Kick / Give host to referees, but IMultiplayerRoomServer.TransferHost() and KickUser() are documented to throw NotHostException for non-host callers. OnlineMultiplayerClient forwards both RPCs directly without additional permission checks. Unless the server authorization was updated in lockstep, referees will encounter runtime failures when attempting these actions.
(Note: Tests like TestKickButtonPresentWhenReferee() verify UI visibility but use TestMultiplayerClient, which is a mock implementation. Actual server-side authorization must be confirmed.)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@osu.Game/Screens/OnlinePlay/Multiplayer/Participants/ParticipantPanel.cs` at
line 298, The UI currently shows host-only actions to referees (kickButton
visibility line uses (client.IsHost || client.IsReferee)), but the server RPCs
(IMultiplayerRoomServer.TransferHost, KickUser) and OnlineMultiplayerClient
forwarders will throw NotHostException for non-host callers; update the
permission checks so referees cannot invoke these RPCs: change the
visibility/enable condition on kickButton and any "give host" control to require
client.IsHost (remove client.IsReferee), and additionally add defensive checks
in the event handlers that call OnlineMultiplayerClient.TransferHost/KickUser to
verify client.IsHost before calling (or catch NotHostException and surface a
user-friendly error) so referees never trigger host-only RPCs.
added 2 commits
April 3, 2026 21:57
) Exposed by CI failures ([example](https://github.com/ppy/osu/actions/runs/23888446400#user-content-r0s0)). The race occurs when a consumer calls `GetBindableDifficulty()` for the first time and then a cache invalidation is triggered. The sequence of events triggering the failure is as follows: 1. Consumer calls `GetBindableDifficulty()` to get a difficulty bindable for a given beatmap tracking the game-global ruleset / mods. This triggers difficulty calculation A. 2. In the meantime, another process requests a cache invalidation for the same beatmap as the one supplied by the consumer in step (1). This incurs a cache purge and triggers difficulty calculation B, but never cancels difficulty calculation A. 3. Difficulty calculation B concludes and writes the correct, latest difficulty value to the bindable. 4. Difficulty calculation A concludes and writes an incorrect, stale difficulty value to the bindable. See below for patch that reproduces this behaviour on my machine 100% reliably: ```diff diff --git a/osu.Game/Beatmaps/BeatmapDifficultyCache.cs b/osu.Game/Beatmaps/BeatmapDifficultyCache.cs index https://github.com/ppy/osu/commit/d6b40639161e26af223f03761b3826b0cd7f4a87..c9604e0e58 100644 --- a/osu.Game/Beatmaps/BeatmapDifficultyCache.cs +++ b/osu.Game/Beatmaps/BeatmapDifficultyCache.cs @@ -252,17 +252,17 @@ private void updateBindable(BindableStarDifficulty bindable, IRulesetInfo? rules GetDifficultyAsync(bindable.BeatmapInfo, rulesetInfo, mods, cancellationToken, computationDelay) .ContinueWith(task => { + StarDifficulty? starDifficulty = task.GetResultSafely(); + // We're on a threadpool thread, but we should exit back to the update thread so consumers can safely handle value-changed events. - Schedule(() => + Scheduler.AddDelayed(() => { if (cancellationToken.IsCancellationRequested) return; - StarDifficulty? starDifficulty = task.GetResultSafely(); - if (starDifficulty != null) bindable.Value = starDifficulty.Value; - }); + }, starDifficulty?.Stars > 0 ? 400 : 0); }, cancellationToken); } ``` The goal of the patch is to reorder the write to the bindable in order to trigger the scenario described above. Due to the invasiveness of the patch it is not suitable to add as a test, and chances are that the schedule delay may need to be tweaked if anyone else wants to check that patch. Anyway, the solution here is to use the same pattern of creating a linked cancellation token even on the first retrieval of a bindable difficulty, and registering it in the list of cancellation tokens that already existed to service the ruleset- / mod-tracking flow. Some extra rearranging in 9184299 is needed to ensure the linked tokens created to do this don't stay behind after they are no longer needed for anything.
Example: https://github.com/ppy/osu/actions/runs/23900675414/job/69696255970?pr=37178#step:5:38 Regressed in #37172, cc @LiquidPL Would fail in multiple tests. I'm not going to spend time figuring out exactly why, I'm just going to guess that not all tests bother to set up the relevant playlist items for the cards or whatever. Some of the failing tests are flaky but not because the `item` here isn't sometimes null in those cases. It's always null, but the callbacks are probably scheduled or whatever and therefore have a chance to never run. Also some of the failures appear to cascade / spill from other tests as well.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@osu.Game/Tests/Visual/Multiplayer/TestMultiplayerClient.cs`:
- Around line 871-873: The patch currently mutates only a cloned settings object
so ServerRoom.Settings.PlaylistItemId remains unchanged; update the server-side
settings before raising the event by assigning PlaylistItemId to
ServerRoom.Settings (e.g. set ServerRoom.Settings.PlaylistItemId = item.ID) and
then call ((IMultiplayerClient)this).SettingsChanged(settings). Ensure you still
pass the cloned/updated settings instance to SettingsChanged but persist the
change on ServerRoom.Settings first so server state is not stale.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: a0e0b9f3-7350-4bce-9f86-bed709e71afa
📒 Files selected for processing (1)
osu.Game/Tests/Visual/Multiplayer/TestMultiplayerClient.cs
Comment on lines
+871
to
+873
| var settings = clone(ServerRoom!.Settings); | ||
| settings.PlaylistItemId = item.ID; | ||
| await ((IMultiplayerClient)this).SettingsChanged(settings).ConfigureAwait(false); |
There was a problem hiding this comment.
Persist server-side PlaylistItemId before raising SettingsChanged.
Line 871–873 only mutates a cloned settings instance. ServerRoom.Settings.PlaylistItemId remains stale, which can later overwrite correct client state when server-side settings are reused.
Proposed fix
- var settings = clone(ServerRoom!.Settings);
- settings.PlaylistItemId = item.ID;
- await ((IMultiplayerClient)this).SettingsChanged(settings).ConfigureAwait(false);
+ ServerRoom!.Settings.PlaylistItemId = item.ID;
+ await ((IMultiplayerClient)this).SettingsChanged(clone(ServerRoom.Settings)).ConfigureAwait(false);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| var settings = clone(ServerRoom!.Settings); | |
| settings.PlaylistItemId = item.ID; | |
| await ((IMultiplayerClient)this).SettingsChanged(settings).ConfigureAwait(false); | |
| ServerRoom!.Settings.PlaylistItemId = item.ID; | |
| await ((IMultiplayerClient)this).SettingsChanged(clone(ServerRoom.Settings)).ConfigureAwait(false); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@osu.Game/Tests/Visual/Multiplayer/TestMultiplayerClient.cs` around lines 871
- 873, The patch currently mutates only a cloned settings object so
ServerRoom.Settings.PlaylistItemId remains unchanged; update the server-side
settings before raising the event by assigning PlaylistItemId to
ServerRoom.Settings (e.g. set ServerRoom.Settings.PlaylistItemId = item.ID) and
then call ((IMultiplayerClient)this).SettingsChanged(settings). Ensure you still
pass the cloned/updated settings instance to SettingsChanged but persist the
change on ServerRoom.Settings first so server state is not stale.
A few quick ones from #37190. ## [Rewrite `TestSceneDeleteLocalScore` to have less context menu containers (and hopefully no longer flake)](ea0bc5b) As seen in https://github.com/ppy/osu/actions/runs/23890777748#user-content-r3s3. This is a speculative fix but I'm feeling somewhat confident about this one. `BeatmapLeaderboardWedge` has TWO separate `ContextMenuContainer`s itself, and the test mentioned here was bringing a third. I have a feeling that the test flaking may have something to do with the fact that the test logic would attempt to click a menu item on specifically ONE of the three context menus. My bet is that when it fails, it's because it's trying the wrong one, but I don't have reproduction. ## [Wait for text to appear in flaking `TestSceneDrawableRoomPlaylist.TestSelectableMouseHandling` test](9d62eea) As seen in https://github.com/ppy/osu/actions/runs/23888446400#user-content-r1s1. Speculative. Banking on the fact that it takes time to load the sprite texts. ## [Remove all tests from `TestSceneWikiMarkdownContainer` dependent on existence of wiki on dev](8f319f9) Deletes a flaky as seen in https://github.com/ppy/osu/actions/runs/23878899702#user-content-r2s2. The year is 2026 and LLM scrapers hammer [the entire](https://sourcehut.org/blog/2025-04-15-you-cannot-have-our-users-data/) [internet](https://blog.metabrainz.org/2025/12/11/we-cant-have-nice-things-because-of-ai-scrapers/) all over to scrape whatever ounce of Human Content there is left to feed the Moloch so that it can regurgitate it back in the form of The Most Average Speech You've Ever Read. We are not immune to this, and as such the LLM homunculi have hit the dev.ppy.sh wiki instance enough times for it to just completely [be banished to the blagole](https://www.youtube.com/watch?v=AfA_2Ku1aJY). Which means I get to freely delete flaky tests that should never have been running as part of CI because they're completely useless now and it's not like we're ever turning them back on again. ## [Use equality check in `TestSceneBeatmapCarouselScrolling.TestScrollPositionMaintainedOnRemove_SecondSelected` that's less sensitive to floating point](6f2a1de) As seen in https://github.com/ppy/osu/actions/runs/23826420794#user-content-r0s1.
There was a problem hiding this comment.
🧹 Nitpick comments (2)
osu.Game.Tests/Visual/SongSelect/TestSceneBeatmapCarouselScrolling.cs (1)
40-45: Good fix for floating-point flakiness.The precision-based comparison correctly addresses potential floating-point comparison issues in UI tests.
Consider applying the same tolerance-based comparison to the similar assertions in the other test methods (
TestScrollPositionMaintainedOnRemove_SecondSelected_WithUserScrollat line 69,TestScrollPositionMaintainedOnRemove_LastSelectedat line 88, etc.) for consistency and to preemptively avoid similar flakiness.,
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@osu.Game.Tests/Visual/SongSelect/TestSceneBeatmapCarouselScrolling.cs` around lines 40 - 45, The precision-based Quad comparison added to the AddAssert in TestSceneBeatmapCarouselScrolling fixes floating-point flakiness; apply the same tolerance-based assertion pattern to the other similar assertions (notably in TestScrollPositionMaintainedOnRemove_SecondSelected_WithUserScroll and TestScrollPositionMaintainedOnRemove_LastSelected) that compare Carousel.ChildrenOfType<PanelBeatmap>().Single(...).ScreenSpaceDrawQuad to positionBefore by replacing exact equality checks with Is.EqualTo(positionBefore).Using<Quad, Quad>((expected, actual) => Precision.AlmostEquals(expected.TopLeft, actual.TopLeft) && Precision.AlmostEquals(expected.TopRight, actual.TopRight) && Precision.AlmostEquals(expected.BottomLeft, actual.BottomLeft) && Precision.AlmostEquals(expected.BottomRight, actual.BottomRight)); ensure you reference the same Selected.Value predicate and positionBefore variable in each case.osu.Game.Tests/Visual/UserInterface/TestSceneDeleteLocalScore.cs (1)
151-156: Consider a less brittle selector than display text for the delete action.At Line 155, finding the menu item by
"delete"text is vulnerable to future label changes. If available, prefer selecting via a stable identifier/action to keep this test resilient.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@osu.Game.Tests/Visual/UserInterface/TestSceneDeleteLocalScore.cs` around lines 151 - 156, The test currently locates the delete menu item by matching its display text via leaderboard.ChildrenOfType<DrawableOsuMenuItem>().First(... Item.Text.Value ... "delete"), which is brittle; instead locate the menu item by a stable identifier or action (e.g. an Action/Id property on the menu item) such as checking DrawableOsuMenuItem.Item.Action or DrawableOsuMenuItem.Item.Id (or a named field you add like Item.Identifier == MenuAction.Delete) and pass that into InputManager.MoveMouseTo; if no stable identifier exists add one to the menu item model and use that in the selector so the test no longer depends on display text.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@osu.Game.Tests/Visual/SongSelect/TestSceneBeatmapCarouselScrolling.cs`:
- Around line 40-45: The precision-based Quad comparison added to the AddAssert
in TestSceneBeatmapCarouselScrolling fixes floating-point flakiness; apply the
same tolerance-based assertion pattern to the other similar assertions (notably
in TestScrollPositionMaintainedOnRemove_SecondSelected_WithUserScroll and
TestScrollPositionMaintainedOnRemove_LastSelected) that compare
Carousel.ChildrenOfType<PanelBeatmap>().Single(...).ScreenSpaceDrawQuad to
positionBefore by replacing exact equality checks with
Is.EqualTo(positionBefore).Using<Quad, Quad>((expected, actual) =>
Precision.AlmostEquals(expected.TopLeft, actual.TopLeft) &&
Precision.AlmostEquals(expected.TopRight, actual.TopRight) &&
Precision.AlmostEquals(expected.BottomLeft, actual.BottomLeft) &&
Precision.AlmostEquals(expected.BottomRight, actual.BottomRight)); ensure you
reference the same Selected.Value predicate and positionBefore variable in each
case.
In `@osu.Game.Tests/Visual/UserInterface/TestSceneDeleteLocalScore.cs`:
- Around line 151-156: The test currently locates the delete menu item by
matching its display text via
leaderboard.ChildrenOfType<DrawableOsuMenuItem>().First(... Item.Text.Value ...
"delete"), which is brittle; instead locate the menu item by a stable identifier
or action (e.g. an Action/Id property on the menu item) such as checking
DrawableOsuMenuItem.Item.Action or DrawableOsuMenuItem.Item.Id (or a named field
you add like Item.Identifier == MenuAction.Delete) and pass that into
InputManager.MoveMouseTo; if no stable identifier exists add one to the menu
item model and use that in the selector so the test no longer depends on display
text.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 841c66d2-fc39-4ad4-a8ac-8bcf9a0e58d4
📒 Files selected for processing (4)
osu.Game.Tests/Visual/Multiplayer/TestSceneDrawableRoomPlaylist.csosu.Game.Tests/Visual/Online/TestSceneWikiMarkdownContainer.csosu.Game.Tests/Visual/SongSelect/TestSceneBeatmapCarouselScrolling.csosu.Game.Tests/Visual/UserInterface/TestSceneDeleteLocalScore.cs
💤 Files with no reviewable changes (1)
- osu.Game.Tests/Visual/Online/TestSceneWikiMarkdownContainer.cs
Kinda self explanatory, adds a second client configurations so its easier to test multiplayer-specific things when using VSCode For people that don't know how to use this, basically just run the first debug like normal, then swap to the second client option and run that. You can also do it in reverse. Visual guide here: https://github.com/user-attachments/assets/1dab50eb-3bd2-422d-a776-852ac4454213
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by Gitar
CatchModSynesthesiato colour catch hit objects based on musical beat divisionalways()to!cancelled()test-resultsjob to report test outcomes with dorny/test-reporterreport-nunit.ymlworkflowThis will update automatically on new commits.
Summary by CodeRabbit
New Features
Tests
Chores