Skip to content

Conversation

@Sheikah45
Copy link
Member

@Sheikah45 Sheikah45 commented Nov 12, 2025

Closes #3434

Summary by CodeRabbit

  • Chores

    • Updated core project commons dependency to a newer release for improved stability and maintenance.
  • Refactor

    • Reworked internal replay file processing to improve memory handling and reliability during replay operations; associated tests updated to match the new processing approach.

@coderabbitai
Copy link

coderabbitai bot commented Nov 12, 2025

Walkthrough

Updated FAF Commons dependency version and replaced ByteBuffer-based raw replay handling with byte[]-based handling in ReplayService and related tests; no public APIs or method signatures were changed.

Changes

Cohort / File(s) Change Summary
Dependency Version Update
build.gradle
Bumped commonsVersion from 20251008-e3f795c to 20251112-0a26247, updating all com.faforever.commons:${commonsVersion} artifacts (data, api, lobby).
ReplayService implementation
src/main/java/com/faforever/client/replay/ReplayService.java
Replaced usage of ByteBuffer (replayData.getData() + ByteBufferBackedInputStream) with byte[] (replayData.getRawReplayData() + ByteArrayInputStream). Added java.io.ByteArrayInputStream import.
ReplayService tests
src/test/java/com/faforever/client/replay/ReplayServiceTest.java
Tests updated to use getRawReplayData() returning byte[] instead of ByteBuffer; removed unused ByteBuffer import and adjusted mock/setup to return raw bytes.
ReplayFileReader tests
src/test/java/com/faforever/client/replay/ReplayFileReaderImplTest.java
Changed assertion from checking getData().capacity() to verifying getRawReplayData().length on parsed replay data.

Sequence Diagram(s)

sequenceDiagram
    participant ReplayService
    participant ReplayData
    participant TempFile

    rect rgb(250, 230, 210)
    Note over ReplayService,ReplayData: Old flow (ByteBuffer)
    ReplayService->>ReplayData: getData()
    ReplayData-->>ReplayService: ByteBuffer
    ReplayService->>ReplayService: ByteBufferBackedInputStream
    ReplayService->>TempFile: write stream -> temp.scfareplay
    end

    rect rgb(210, 245, 230)
    Note over ReplayService,ReplayData: New flow (byte[])
    ReplayService->>ReplayData: getRawReplayData()
    ReplayData-->>ReplayService: byte[]
    ReplayService->>ReplayService: ByteArrayInputStream
    ReplayService->>TempFile: write stream -> temp.scfareplay
    end
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

  • Areas to check:
    • Ensure getRawReplayData() semantics and null/empty handling match previous ByteBuffer behavior.
    • Verify temp file write preserves full replay contents and ordering.
    • Confirm tests cover edge cases and adjust any remaining ByteBuffer usages.

Poem

🐰 I nibble on bytes both small and new,
Swapped buffers for arrays — tidy and true.
A hop, a patch, a replay that sings,
Temp files written, joy it brings.
Hooray — the replays dance anew!

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately describes the main change: updating java-commons to fix a replay parser bug, which aligns with the core purpose of this PR.
Linked Issues check ✅ Passed The PR updates java-commons and modifies replay parsing code to restore replay functionality after a regression, directly addressing issue #3434's requirement to fix replay loading failures.
Out of Scope Changes check ✅ Passed All changes are focused on replay parsing: updating commons version, switching from ByteBuffer to byte[] in ReplayService, and updating corresponding tests. No extraneous modifications detected.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch bugfix/replay-parser

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 64389be and 9d5786e.

📒 Files selected for processing (4)
  • build.gradle (1 hunks)
  • src/main/java/com/faforever/client/replay/ReplayService.java (2 hunks)
  • src/test/java/com/faforever/client/replay/ReplayFileReaderImplTest.java (1 hunks)
  • src/test/java/com/faforever/client/replay/ReplayServiceTest.java (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
  • src/main/java/com/faforever/client/replay/ReplayService.java
  • build.gradle
⏰ 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). (2)
  • GitHub Check: checks
  • GitHub Check: checks
🔇 Additional comments (2)
src/test/java/com/faforever/client/replay/ReplayFileReaderImplTest.java (1)

31-31: LGTM! Correctly updated to match the new API.

The assertion properly validates the raw replay data length using the new getRawReplayData().length method, replacing the previous ByteBuffer-based getData().capacity() approach. This aligns with the commons library update that fixes the replay parser bug.

src/test/java/com/faforever/client/replay/ReplayServiceTest.java (1)

188-188: LGTM! Mock setup correctly updated.

The mock configuration properly returns the raw byte array via getRawReplayData(), replacing the previous ByteBuffer-based getData() approach. This ensures all test methods using this mock will work correctly with the updated commons library API.


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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@Sheikah45 Sheikah45 enabled auto-merge (squash) November 12, 2025 11:32
@Sheikah45 Sheikah45 force-pushed the bugfix/replay-parser branch from 44b5f8c to 9d5786e Compare November 12, 2025 23:59
@Sheikah45 Sheikah45 merged commit 5456695 into develop Nov 13, 2025
4 checks passed
@Sheikah45 Sheikah45 deleted the bugfix/replay-parser branch November 13, 2025 00:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug]: Unable to view replays after 2025.11.0 update

2 participants