Reduce retained binary memory with range-reader groundwork#742
Open
Reduce retained binary memory with range-reader groundwork#742
Conversation
📝 WalkthroughWalkthroughAdds deferred binary-content loading and container-format detection: new reader/loader abstractions and range-based file reads; refactors file factories and file classes (ELF/PE/RAW) to support lazy loading, and updates UI and tests to use the new lazy APIs. Changes
sequenceDiagram
participant Caller
participant AbstractFile.Companion as Factory
participant FileSystem as BinaryRangeReader
participant Parser as ELF/PE/RAW
Caller->>Factory: createInstance(file)
Factory->>FileSystem: detectBinaryContainerFormat(file)
FileSystem-->>Factory: ELF / PE / RAW
Factory->>Parser: instantiate appropriate File (may pass loader)
alt PE needs parsing bytes
Factory->>FileSystem: readFileContentsForParsing(file)
FileSystem-->>Factory: parsing bytes
Factory->>Parser: provide bytes (or deferred loader)
end
Parser-->>Caller: AbstractFile instance (deferred or preloaded)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 Tip CodeRabbit can generate a title for your PR based on the changes.Add |
Owner
Author
|
Added another instrumentation-focused commit on top of the range-reader work:
Verification:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (1)
app/src/main/java/com/kyhsgeekcode/disassembler/files/AbstractFile.kt (1)
131-133: Drop the unused eager read in the Unity DLL path.Line 132 reads the entire file into memory, but nothing in this branch consumes
content. That partially undoes the PR's memory savings on one of the larger binaries this code handles.♻️ Proposed cleanup
if (file.path.endsWith("assets/bin/Data/Managed/Assembly-CSharp.dll")) { // Unity C# dll file - val content = readFileContentsForParsing(file) Logger.v(TAG, "Found C# unity dll") try {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/java/com/kyhsgeekcode/disassembler/files/AbstractFile.kt` around lines 131 - 133, The branch that checks file.path.endsWith("assets/bin/Data/Managed/Assembly-CSharp.dll") eagerly calls readFileContentsForParsing(file) and assigns it to content but never uses it, negating memory savings; remove the unused call and variable so the branch only logs via Logger.v(TAG, "Found C# unity dll") (or any remaining necessary logic) and rely on lazy parsing where needed, ensuring no other code in the Assembly-CSharp.dll branch depends on the removed content variable or call.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/src/main/java/com/kyhsgeekcode/disassembler/files/AbstractFile.kt`:
- Around line 23-33: The toString() method incorrectly treats an uninitialized
deferred property fileContents as "not configured" and returns a placeholder;
remove that early ::fileContents.isInitialized check and let toString() always
emit available metadata (use getBinaryLength(), parsed fields) even when
fileContents is lazy-loaded; if RawFile still needs a special prefix, gate only
that prefix on the concrete type (this is RawFile) rather than on fileContents
initialization, ensuring ElfFile/PEFile and others show size/address info
regardless of fileContents state.
In
`@app/src/main/java/com/kyhsgeekcode/disassembler/files/FileChannelBinaryRangeReader.kt`:
- Around line 15-35: The read(offset: Long, length: Int) implementation assumes
channel.read(buffer, offset) fills the buffer; change it to handle short reads
by looping on channel.read until the buffer has no remaining space or EOF (-1)
is returned, accumulating the total bytes read and stopping when reached
readableLength; then return only the bytes actually read (use buffer.position()
or similar) so partial reads don't leave zero-padded data. Update readFully() to
rely on the corrected read(...) behavior for large reads and ensure you respect
the readableLength computed in read(offset,length) and break the loop on -1 to
avoid infinite loops.
In `@app/src/main/java/com/kyhsgeekcode/disassembler/files/PEFile.java`:
- Around line 36-48: The constructor PEFile(File file, byte[] filec,
BinaryContentLoader deferredContentLoader) sets contentLoaded = true when
deferredContentLoader is null but never assigns fileContents, leaving
getBinaryContents()/getBinaryLength() to read a null buffer; fix by initializing
the instance field fileContents with parsingBytes (or a copy) before setting
contentLoaded when deferredContentLoader == null (inside the same constructor),
ensuring DeferredFileBackedBinaryContent is created as before and then
fileContents is populated so getBinaryContents()/getBinaryLength() are safe.
In `@app/src/main/java/com/kyhsgeekcode/disassembler/files/RawFile.kt`:
- Around line 10-15: The constructor sets contentLoaded = filecontent != null
but never initializes the superclass lateinit fileContents, so calls to
getBinaryContents() or getBinaryLength() will crash on the eager-content path;
modify the RawFile constructor to assign fileContents when filecontent is
non-null (e.g., wrap or extract the initialContent into the same structure
DeferredFileBackedBinaryContent expects or set fileContents directly) and ensure
binaryContent and DeferredFileBackedBinaryContent initialization uses that same
initialized fileContents state; update the constructor code paths around
contentLoaded, binaryContent, and initialization of fileContents so both eager
(filecontent != null) and deferred (deferredContentLoader) cases populate
fileContents consistently.
In `@app/src/main/java/com/kyhsgeekcode/disassembler/ui/tabs/BinaryDisasmTab.kt`:
- Around line 159-160: The isValidAddress function currently checks addresses
against a mix of file offsets and virtual base; update it to validate jump
targets against the mapped virtual-address range by ensuring address >=
file.codeVirtAddr and address < file.codeVirtAddr + file.getBinaryLength() (end
exclusive) so jumpto() and loadMore() cannot receive unmapped addresses; adjust
the check in isValidAddress accordingly and ensure any callers (e.g., jumpto(),
loadMore()) rely on this updated predicate.
---
Nitpick comments:
In `@app/src/main/java/com/kyhsgeekcode/disassembler/files/AbstractFile.kt`:
- Around line 131-133: The branch that checks
file.path.endsWith("assets/bin/Data/Managed/Assembly-CSharp.dll") eagerly calls
readFileContentsForParsing(file) and assigns it to content but never uses it,
negating memory savings; remove the unused call and variable so the branch only
logs via Logger.v(TAG, "Found C# unity dll") (or any remaining necessary logic)
and rely on lazy parsing where needed, ensuring no other code in the
Assembly-CSharp.dll branch depends on the removed content variable or call.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 18960237-4633-4b97-80bc-572e9c8feb41
📒 Files selected for processing (15)
app/src/main/java/com/kyhsgeekcode/disassembler/files/AbstractFile.ktapp/src/main/java/com/kyhsgeekcode/disassembler/files/BinaryContainerFormat.ktapp/src/main/java/com/kyhsgeekcode/disassembler/files/BinaryContentLoader.ktapp/src/main/java/com/kyhsgeekcode/disassembler/files/BinaryRangeReader.ktapp/src/main/java/com/kyhsgeekcode/disassembler/files/DeferredFileBackedBinaryContent.ktapp/src/main/java/com/kyhsgeekcode/disassembler/files/ElfFile.ktapp/src/main/java/com/kyhsgeekcode/disassembler/files/FileChannelBinaryRangeReader.ktapp/src/main/java/com/kyhsgeekcode/disassembler/files/PEFile.javaapp/src/main/java/com/kyhsgeekcode/disassembler/files/RawFile.ktapp/src/main/java/com/kyhsgeekcode/disassembler/ui/tabs/BinaryDisasmTab.ktapp/src/test/java/com/kyhsgeekcode/disassembler/files/AbstractFileReaderBridgeTest.ktapp/src/test/java/com/kyhsgeekcode/disassembler/files/BinaryContainerFormatDetectionTest.ktapp/src/test/java/com/kyhsgeekcode/disassembler/files/DeferredFileBackedBinaryContentTest.ktapp/src/test/java/com/kyhsgeekcode/disassembler/files/FileChannelBinaryRangeReaderTest.ktapp/src/test/java/com/kyhsgeekcode/disassembler/files/RawFileLazyContentTest.kt
Comment on lines
23
to
33
| override fun toString(): String { | ||
| if (fileContents == null) { | ||
| if (!::fileContents.isInitialized) { | ||
| return "The file has not been configured. You should setup manually in the first page before you can see the details." | ||
| } | ||
| val builder = StringBuilder( | ||
| if (this is RawFile) "The file has not been configured. You should setup manually in the first page before you can see the details." + | ||
| System.lineSeparator() else "" | ||
| ) | ||
| builder.append(/*R.getString(R.string.FileSize)*/"File Size:") | ||
| .append(Integer.toHexString(fileContents.size)) | ||
| .append(java.lang.Long.toHexString(getBinaryLength())) | ||
| .append(ls) |
There was a problem hiding this comment.
Don't treat deferred bytes as "not configured".
Line 24 now returns the placeholder string whenever fileContents is still lazy. After this PR, RawFile, ElfFile, and PEFile intentionally leave fileContents uninitialized until bytes are requested, so toString() skips the size/address metadata even though getBinaryLength() and the parsed fields are already available.
🩹 Proposed fix
override fun toString(): String {
- if (!::fileContents.isInitialized) {
- return "The file has not been configured. You should setup manually in the first page before you can see the details."
- }
val builder = StringBuilder(
if (this is RawFile) "The file has not been configured. You should setup manually in the first page before you can see the details." +
System.lineSeparator() else ""
)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/src/main/java/com/kyhsgeekcode/disassembler/files/AbstractFile.kt` around
lines 23 - 33, The toString() method incorrectly treats an uninitialized
deferred property fileContents as "not configured" and returns a placeholder;
remove that early ::fileContents.isInitialized check and let toString() always
emit available metadata (use getBinaryLength(), parsed fields) even when
fileContents is lazy-loaded; if RawFile still needs a special prefix, gate only
that prefix on the concrete type (this is RawFile) rather than on fileContents
initialization, ensuring ElfFile/PEFile and others show size/address info
regardless of fileContents state.
Comment on lines
+15
to
+35
| override fun read(offset: Long, length: Int): ByteArray { | ||
| require(offset >= 0) { "offset must be non-negative" } | ||
| require(length >= 0) { "length must be non-negative" } | ||
| if (length == 0 || offset >= size) { | ||
| return ByteArray(0) | ||
| } | ||
| val readableLength = min(length.toLong(), size - offset).toInt() | ||
| val buffer = ByteBuffer.allocate(readableLength) | ||
| channel.read(buffer, offset) | ||
| return buffer.array() | ||
| } | ||
|
|
||
| override fun readFully(): ByteArray { | ||
| if (size == 0L) { | ||
| return ByteArray(0) | ||
| } | ||
| require(size <= Int.MAX_VALUE) { | ||
| "File is too large to fit into a ByteArray: $size bytes" | ||
| } | ||
| return read(offset = 0, length = size.toInt()) | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Does java.nio.channels.FileChannel.read(ByteBuffer, long) guarantee that a single call fills the ByteBuffer for regular files, or can it legally return fewer bytes than requested?
💡 Result:
No, java.nio.channels.FileChannel.read(ByteBuffer, long) does not guarantee that a single call fills the ByteBuffer, even for regular files. It can legally return fewer bytes than requested.
Citations:
- 1: https://docs.oracle.com/en/java/javase/23/docs/api/java.base/java/nio/channels/FileChannel.html
- 2: https://docs.oracle.com/javase/8/docs/api/java/nio/channels/FileChannel.html
- 3: https://docs.oracle.com/en/java/javase/17/docs/api/java.base/java/nio/channels/ReadableByteChannel.html
- 4: https://cr.openjdk.org/~alanb/api/java.base/java/nio/channels/FileChannel.html
- 5: https://stackoverflow.com/questions/3967147/would-filechannel-read-read-less-bytes-than-specified-if-theres-enough-data
- 6: https://docs.oracle.com/javase/8/docs/api/java/nio/channels/ReadableByteChannel.html
- 7: https://docs.oracle.com/en/java/javase/13/docs/api/java.base/java/nio/channels/ReadableByteChannel.html
Handle short reads instead of assuming one FileChannel.read fills the buffer.
The channel.read(buffer, offset) call at line 23 ignores the returned byte count. Since FileChannel.read() can legally return fewer bytes than requested, even for regular files, a short read leaves the buffer partially uninitialized (zero-padded). This corrupts readFully() at line 34, which inherits the same defect.
Use a loop to read until the buffer is full or EOF:
Proposed fix
override fun read(offset: Long, length: Int): ByteArray {
require(offset >= 0) { "offset must be non-negative" }
require(length >= 0) { "length must be non-negative" }
if (length == 0 || offset >= size) {
return ByteArray(0)
}
val readableLength = min(length.toLong(), size - offset).toInt()
val buffer = ByteBuffer.allocate(readableLength)
- channel.read(buffer, offset)
- return buffer.array()
+ var position = offset
+ while (buffer.hasRemaining()) {
+ val bytesRead = channel.read(buffer, position)
+ if (bytesRead <= 0) {
+ break
+ }
+ position += bytesRead
+ }
+ return buffer.array().copyOf(buffer.position())
}📝 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
| override fun read(offset: Long, length: Int): ByteArray { | |
| require(offset >= 0) { "offset must be non-negative" } | |
| require(length >= 0) { "length must be non-negative" } | |
| if (length == 0 || offset >= size) { | |
| return ByteArray(0) | |
| } | |
| val readableLength = min(length.toLong(), size - offset).toInt() | |
| val buffer = ByteBuffer.allocate(readableLength) | |
| channel.read(buffer, offset) | |
| return buffer.array() | |
| } | |
| override fun readFully(): ByteArray { | |
| if (size == 0L) { | |
| return ByteArray(0) | |
| } | |
| require(size <= Int.MAX_VALUE) { | |
| "File is too large to fit into a ByteArray: $size bytes" | |
| } | |
| return read(offset = 0, length = size.toInt()) | |
| } | |
| override fun read(offset: Long, length: Int): ByteArray { | |
| require(offset >= 0) { "offset must be non-negative" } | |
| require(length >= 0) { "length must be non-negative" } | |
| if (length == 0 || offset >= size) { | |
| return ByteArray(0) | |
| } | |
| val readableLength = min(length.toLong(), size - offset).toInt() | |
| val buffer = ByteBuffer.allocate(readableLength) | |
| var position = offset | |
| while (buffer.hasRemaining()) { | |
| val bytesRead = channel.read(buffer, position) | |
| if (bytesRead <= 0) { | |
| break | |
| } | |
| position += bytesRead | |
| } | |
| return buffer.array().copyOf(buffer.position()) | |
| } | |
| override fun readFully(): ByteArray { | |
| if (size == 0L) { | |
| return ByteArray(0) | |
| } | |
| require(size <= Int.MAX_VALUE) { | |
| "File is too large to fit into a ByteArray: $size bytes" | |
| } | |
| return read(offset = 0, length = size.toInt()) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@app/src/main/java/com/kyhsgeekcode/disassembler/files/FileChannelBinaryRangeReader.kt`
around lines 15 - 35, The read(offset: Long, length: Int) implementation assumes
channel.read(buffer, offset) fills the buffer; change it to handle short reads
by looping on channel.read until the buffer has no remaining space or EOF (-1)
is returned, accumulating the total bytes read and stopping when reached
readableLength; then return only the bytes actually read (use buffer.position()
or similar) so partial reads don't leave zero-padded data. Update readFully() to
rely on the corrected read(...) behavior for large reads and ensure you respect
the readableLength computed in read(offset,length) and break the loop on -1 to
avoid infinite loops.
Comment on lines
36
to
+48
| public PEFile(File file, byte[] filec) throws IOException, NotThisFormatException { | ||
| this(file, filec, null); | ||
| } | ||
|
|
||
| public PEFile(File file, byte[] filec, BinaryContentLoader deferredContentLoader) throws IOException, NotThisFormatException { | ||
| path = file.getPath(); | ||
| byte[] parsingBytes = filec != null ? filec : loadBinaryContents(file, deferredContentLoader); | ||
| binaryContent = new DeferredFileBackedBinaryContent( | ||
| file, | ||
| deferredContentLoader == null ? parsingBytes : null, | ||
| deferredContentLoader | ||
| ); | ||
| contentLoaded = deferredContentLoader == null; |
There was a problem hiding this comment.
Initialize fileContents before marking the PE payload as loaded.
When deferredContentLoader is null, Line 48 sets contentLoaded = true, but no value is assigned to fileContents. Any caller that still uses the two-arg constructor on Line 36 will then hit an uninitialized/null buffer in getBinaryContents() and getBinaryLength().
🐛 Proposed fix
byte[] parsingBytes = filec != null ? filec : loadBinaryContents(file, deferredContentLoader);
binaryContent = new DeferredFileBackedBinaryContent(
file,
deferredContentLoader == null ? parsingBytes : null,
deferredContentLoader
);
- contentLoaded = deferredContentLoader == null;
+ if (deferredContentLoader == null) {
+ fileContents = parsingBytes;
+ contentLoaded = true;
+ } else {
+ contentLoaded = false;
+ }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/src/main/java/com/kyhsgeekcode/disassembler/files/PEFile.java` around
lines 36 - 48, The constructor PEFile(File file, byte[] filec,
BinaryContentLoader deferredContentLoader) sets contentLoaded = true when
deferredContentLoader is null but never assigns fileContents, leaving
getBinaryContents()/getBinaryLength() to read a null buffer; fix by initializing
the instance field fileContents with parsingBytes (or a copy) before setting
contentLoaded when deferredContentLoader == null (inside the same constructor),
ensuring DeferredFileBackedBinaryContent is created as before and then
fileContents is populated so getBinaryContents()/getBinaryLength() are safe.
Comment on lines
+10
to
+15
| private var contentLoaded = filecontent != null | ||
| private val binaryContent = DeferredFileBackedBinaryContent( | ||
| file = file, | ||
| initialContent = filecontent, | ||
| loader = deferredContentLoader?.let { BinaryContentLoader { it() } }, | ||
| ) |
There was a problem hiding this comment.
Initialize fileContents on the eager-content path.
contentLoaded becomes true as soon as filecontent is non-null, but this constructor never assigns the superclass fileContents. The first getBinaryContents() or getBinaryLength() call on that path will therefore hit an uninitialized lateinit property and throw.
Suggested fix
init {
path = file.path
+ if (filecontent != null) {
+ fileContents = filecontent
+ }
}Also applies to: 17-30
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/src/main/java/com/kyhsgeekcode/disassembler/files/RawFile.kt` around
lines 10 - 15, The constructor sets contentLoaded = filecontent != null but
never initializes the superclass lateinit fileContents, so calls to
getBinaryContents() or getBinaryLength() will crash on the eager-content path;
modify the RawFile constructor to assign fileContents when filecontent is
non-null (e.g., wrap or extract the initialContent into the same structure
DeferredFileBackedBinaryContent expects or set fileContents directly) and ensure
binaryContent and DeferredFileBackedBinaryContent initialization uses that same
initialized fileContents state; update the constructor code paths around
contentLoaded, binaryContent, and initialization of fileContents so both eager
(filecontent != null) and deferred (deferredContentLoader) cases populate
fileContents consistently.
Comment on lines
159
to
+160
| private fun isValidAddress(address: Long): Boolean { | ||
| return if (address > file.fileContents.size + file.codeVirtAddr) false else address >= 0 | ||
| return if (address > file.getBinaryLength() + file.codeVirtAddr) false else address >= 0 |
There was a problem hiding this comment.
Validate jump targets against the mapped virtual-address range.
Now that the upper bound is in virtual-address space, the lower bound has to move to file.codeVirtAddr as well, and the end should be exclusive. As written, this still accepts addresses below the image base and codeVirtAddr + getBinaryLength(), so jumpto() can hand loadMore() an unmapped address.
Suggested fix
private fun isValidAddress(address: Long): Boolean {
- return if (address > file.getBinaryLength() + file.codeVirtAddr) false else address >= 0
+ val start = file.codeVirtAddr
+ val endExclusive = start + file.getBinaryLength()
+ return address >= start && address < endExclusive
}📝 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
| private fun isValidAddress(address: Long): Boolean { | |
| return if (address > file.fileContents.size + file.codeVirtAddr) false else address >= 0 | |
| return if (address > file.getBinaryLength() + file.codeVirtAddr) false else address >= 0 | |
| private fun isValidAddress(address: Long): Boolean { | |
| val start = file.codeVirtAddr | |
| val endExclusive = start + file.getBinaryLength() | |
| return address >= start && address < endExclusive | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/src/main/java/com/kyhsgeekcode/disassembler/ui/tabs/BinaryDisasmTab.kt`
around lines 159 - 160, The isValidAddress function currently checks addresses
against a mix of file offsets and virtual base; update it to validate jump
targets against the mapped virtual-address range by ensuring address >=
file.codeVirtAddr and address < file.codeVirtAddr + file.getBinaryLength() (end
exclusive) so jumpto() and loadMore() cannot receive unmapped addresses; adjust
the check in isValidAddress accordingly and ensure any callers (e.g., jumpto(),
loadMore()) rely on this updated predicate.
There was a problem hiding this comment.
🧹 Nitpick comments (2)
app/src/main/java/com/kyhsgeekcode/disassembler/ui/tabs/BinaryTab.kt (1)
240-245: Prefer explicit stable tags over::class.simpleNamefallback.At Line 243,
binary_tab_${tabKind::class.simpleName}is brittle for test selectors (renames/obfuscation). Consider a fixed mapping per tab kind.♻️ Suggested stable mapping
private fun binaryTabTestTag(tabKind: BinaryTabKind): String { return when (tabKind) { - is BinaryTabKind.BinaryDetail -> com.kyhsgeekcode.disassembler.ui.MainTestTags.BINARY_TAB_DETAIL - else -> "binary_tab_${tabKind::class.simpleName}" + is BinaryTabKind.BinaryDetail -> com.kyhsgeekcode.disassembler.ui.MainTestTags.BINARY_TAB_DETAIL + is BinaryTabKind.BinaryOverview -> "binary_tab_overview" + is BinaryTabKind.BinaryImportSymbol -> "binary_tab_import_symbol" + is BinaryTabKind.BinaryExportSymbol -> "binary_tab_export_symbol" + is BinaryTabKind.BinaryDisasm -> "binary_tab_disassembly" + is BinaryTabKind.BinaryString -> "binary_tab_strings" } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/java/com/kyhsgeekcode/disassembler/ui/tabs/BinaryTab.kt` around lines 240 - 245, The binaryTabTestTag function uses a fragile fallback "binary_tab_${tabKind::class.simpleName}" for test selectors; update binaryTabTestTag to return explicit, stable tag strings for each BinaryTabKind variant instead of relying on ::class.simpleName (handle BinaryTabKind.BinaryDetail already, and add explicit cases for other variants of BinaryTabKind or a sealed when-else that returns constants), and preferably reference or add descriptive constants (e.g., in MainTestTags) so test selectors remain stable across renames/obfuscation.app/src/androidTest/java/com/kyhsgeekcode/disassembler/MainActivityActionViewProjectArchiveIntentFlowTest.kt (1)
54-62: Avoid singleton-coupled assertions in the recreate test.Line 54 and Line 60 gate on
ProjectManager.currentProject, but that field is process-singleton state (ProjectManager.currentProjectinapp/src/main/java/com/kyhsgeekcode/disassembler/project/ProjectManager.kt:35-59), so this can pass without validating true recreation restoration.Use UI/state observable from the recreated activity as the primary readiness/assertion signal.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/androidTest/java/com/kyhsgeekcode/disassembler/MainActivityActionViewProjectArchiveIntentFlowTest.kt` around lines 54 - 62, The test currently uses process-singleton state ProjectManager.currentProject around composeRule.activityRule.scenario.recreate(), which can hide failures; replace those waits that reference ProjectManager.currentProject with UI/state observations from the recreated activity instead (keep the recreate call composeRule.activityRule.scenario.recreate() as-is). Concretely, change the two composeRule.waitUntil blocks that check ProjectManager.currentProject to wait/assert on a Compose UI node that reflects the opened project (e.g., use composeRule.waitUntil or composeRule.onNodeWithText("ArchiveFixture").assertExists() or check a semantics tag like "projectName" via composeRule.onNodeWithTag("projectName").assertExists()) so the test verifies the recreated Activity/Compose state rather than the singleton ProjectManager. Ensure both pre- and post-recreate readiness/assertions use the UI-based check.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@app/src/androidTest/java/com/kyhsgeekcode/disassembler/MainActivityActionViewProjectArchiveIntentFlowTest.kt`:
- Around line 54-62: The test currently uses process-singleton state
ProjectManager.currentProject around
composeRule.activityRule.scenario.recreate(), which can hide failures; replace
those waits that reference ProjectManager.currentProject with UI/state
observations from the recreated activity instead (keep the recreate call
composeRule.activityRule.scenario.recreate() as-is). Concretely, change the two
composeRule.waitUntil blocks that check ProjectManager.currentProject to
wait/assert on a Compose UI node that reflects the opened project (e.g., use
composeRule.waitUntil or
composeRule.onNodeWithText("ArchiveFixture").assertExists() or check a semantics
tag like "projectName" via
composeRule.onNodeWithTag("projectName").assertExists()) so the test verifies
the recreated Activity/Compose state rather than the singleton ProjectManager.
Ensure both pre- and post-recreate readiness/assertions use the UI-based check.
In `@app/src/main/java/com/kyhsgeekcode/disassembler/ui/tabs/BinaryTab.kt`:
- Around line 240-245: The binaryTabTestTag function uses a fragile fallback
"binary_tab_${tabKind::class.simpleName}" for test selectors; update
binaryTabTestTag to return explicit, stable tag strings for each BinaryTabKind
variant instead of relying on ::class.simpleName (handle
BinaryTabKind.BinaryDetail already, and add explicit cases for other variants of
BinaryTabKind or a sealed when-else that returns constants), and preferably
reference or add descriptive constants (e.g., in MainTestTags) so test selectors
remain stable across renames/obfuscation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4d4d1e33-6dc0-46d6-80e5-3b06076439da
📒 Files selected for processing (7)
app/src/androidTest/java/com/kyhsgeekcode/disassembler/MainActivityActionViewProjectArchiveIntentFlowTest.ktapp/src/androidTest/java/com/kyhsgeekcode/disassembler/MainActivityAdvancedImportCancelFlowTest.ktapp/src/androidTest/java/com/kyhsgeekcode/disassembler/MainActivityBinaryDetailExportFlowTest.ktapp/src/androidTest/java/com/kyhsgeekcode/disassembler/MainActivitySafImportCancelFlowTest.ktapp/src/main/java/com/kyhsgeekcode/disassembler/ui/MainTestTags.ktapp/src/main/java/com/kyhsgeekcode/disassembler/ui/tabs/BinaryDetailTab.ktapp/src/main/java/com/kyhsgeekcode/disassembler/ui/tabs/BinaryTab.kt
✅ Files skipped from review due to trivial changes (1)
- app/src/main/java/com/kyhsgeekcode/disassembler/ui/MainTestTags.kt
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
1 participant
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
FileChannel-backed binary range reader abstraction for future large-file refactorsAbstractFile.createInstance()behind a reader bridge instead of callingFile.readBytes()directlyWhat Changed
BinaryRangeReaderandFileChannelBinaryRangeReaderDeferredFileBackedBinaryContentand a shared loader interfaceRawFileandElfFilecontents deferred until a view actually needs bytesPEFilewith a temporary byte array but avoid keeping that array attached to the object afterwardTest Coverage
Verification
JAVA_HOME=$(/usr/libexec/java_home -v 17) ANDROID_HOME=$HOME/Library/Android/sdk ./gradlew testDebugUnitTest assembleDebugSummary by CodeRabbit
New Features
Performance Improvements
Bug Fixes
Tests