stability: reduce string-search pressure on large files#741
Open
stability: reduce string-search pressure on large files#741
Conversation
This was referenced Mar 21, 2026
|
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:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (6)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAnalyzer now caps emitted detected strings at 4,096 chars; StringTab enforces per-result (4,096) and global rendered-character (32,768) budgets, scans only a preview of input, and surfaces a truncation notice. MainViewModel intent selection was refactored to an IncomingSelection resolver and unified project-open state application. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Analyzer
participant Callback as AnalyzerCallback
participant Accumulator as StringSearchResultAccumulator
participant UI as StringTabData
User->>Analyzer: searchStrings(start, end)
Analyzer->>Analyzer: detect candidate string
Analyzer->>Analyzer: truncate to MAX_EMITTED_FOUND_STRING_CHARS (4096)
Analyzer->>Callback: emit FoundString (clipped)
Callback->>Accumulator: append(found)
Accumulator->>Accumulator: clip per-result to MAX_RENDERED_STRING_CHARS (4096)
Accumulator->>Accumulator: enforce global budget MAX_RENDERED_STRING_TOTAL_CHARS (32768)
Accumulator->>Accumulator: set isTruncated when budget exhausted
Accumulator-->>UI: provide clipped results + truncation flag
UI->>UI: build notice from input/result truncation and render
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
app/src/main/java/com/kyhsgeekcode/disassembler/ui/tabs/StringTab.kt (1)
130-140: Clarify: Length column shows original length, not displayed string length.When a string is clipped, the "Length" column (line 136) displays
item.length(the original string length from the analyzer), while the "String" column shows the truncated content. This creates an intentional but potentially confusing mismatch—e.g., "Length: 5000" with only 4,093 visible characters + "...".If this is the intended UX to help users understand truncation occurred, consider adding a visual indicator (e.g., a truncation icon or tooltip) to make the behavior explicit.
🤖 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/StringTab.kt` around lines 130 - 140, The Length column currently shows the analyzer's original length (item.length) while the String column shows a truncated string (item.string), which can confuse users; update the column renderer in StringTab.kt (the when branch for col == 1 inside the table lambda) to either display the visible/truncated length (item.string.length) or append the original length (e.g., "${item.string.length} (orig ${item.length})"), and add a visual indicator for truncation by detecting when item.string.length < item.length (e.g., show a tooltip or truncation icon on the String cell or Length cell that exposes the full original length). Ensure the change references the same items list and keys and only modifies the col==1 and col==2 rendering logic to surface truncation info.
🤖 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/main/java/com/kyhsgeekcode/disassembler/ui/tabs/StringTab.kt`:
- Around line 130-140: The Length column currently shows the analyzer's original
length (item.length) while the String column shows a truncated string
(item.string), which can confuse users; update the column renderer in
StringTab.kt (the when branch for col == 1 inside the table lambda) to either
display the visible/truncated length (item.string.length) or append the original
length (e.g., "${item.string.length} (orig ${item.length})"), and add a visual
indicator for truncation by detecting when item.string.length < item.length
(e.g., show a tooltip or truncation icon on the String cell or Length cell that
exposes the full original length). Ensure the change references the same items
list and keys and only modifies the col==1 and col==2 rendering logic to surface
truncation info.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9281b105-7799-474a-b7de-1a6621c2da00
📒 Files selected for processing (4)
app/src/main/java/com/kyhsgeekcode/disassembler/Analyzer.ktapp/src/main/java/com/kyhsgeekcode/disassembler/ui/tabs/StringTab.ktapp/src/test/java/com/kyhsgeekcode/disassembler/AnalyzerStringSearchTest.ktapp/src/test/java/com/kyhsgeekcode/disassembler/ui/tabs/StringSearchResultAccumulatorTest.kt
Owner
Author
|
Added another TDD slice on top of #741. This follow-up now bounds string-search scans themselves, not just rendered result payloads: StringTab searches only a bounded preview for very large files and surfaces a notice when the scan is limited. That keeps string search aligned with the bounded preview strategy already used in Text/Hex/Analysis tabs. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
app/src/main/java/com/kyhsgeekcode/disassembler/ui/tabs/StringTab.kt (1)
40-49: Derive truncation from the bytes you actually search.
isTruncated = originalSize > maxBytesassumes every caller hands this helper exactlymaxByteswhen the scan is bounded. IfpreviewBytesis ever shorter for any other reason,Analyzerwill still search a prefix but the UI will say it was unbounded. Basing the flag onpreviewBytes.sizekeeps the notice aligned with the real input.♻️ Suggested tweak
internal fun buildStringSearchInput( previewBytes: ByteArray, originalSize: Long, maxBytes: Int = MAX_SEARCHED_STRING_BYTES ): StringSearchInput { + val boundedBytes = if (previewBytes.size > maxBytes) { + previewBytes.copyOf(maxBytes) + } else { + previewBytes + } + return StringSearchInput( - bytes = previewBytes, + bytes = boundedBytes, originalSize = originalSize, - isTruncated = originalSize > maxBytes + isTruncated = boundedBytes.size.toLong() < originalSize ) }🤖 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/StringTab.kt` around lines 40 - 49, The isTruncated flag in buildStringSearchInput is computed from originalSize and maxBytes which can be incorrect if previewBytes is shorter for other reasons; change the computation in buildStringSearchInput to set isTruncated based on previewBytes.size vs originalSize (e.g., isTruncated = previewBytes.size < originalSize) so the flag reflects the actual bytes being searched (refer to buildStringSearchInput, StringSearchInput, previewBytes, originalSize).
🤖 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/ui/tabs/StringTab.kt`:
- Around line 52-67: buildStringSearchNotice currently conflates two different
truncation causes (row-count truncation and content clipping) because
input.isTruncated is overloaded; change the function to accept two distinct
booleans (e.g., countTruncated: Boolean, contentClipped: Boolean) or otherwise
distinguish input.isTruncated into those two signals at the caller sites (places
around the previous comments ~99-117 and ~151-152), then build separate notice
parts: when countTruncated is true append the existing "Showing first
$MAX_RENDERED_STRING_RESULTS results." message, and when contentClipped is true
append a message indicating rendering/content clipping (e.g., "Some results were
truncated by the render-character budget" and, if available, include analyzer
clipping like "analyzer truncated to 4096 chars"); update callers of
buildStringSearchNotice and any uses of input.isTruncated so the UI accurately
reflects count vs content clipping.
---
Nitpick comments:
In `@app/src/main/java/com/kyhsgeekcode/disassembler/ui/tabs/StringTab.kt`:
- Around line 40-49: The isTruncated flag in buildStringSearchInput is computed
from originalSize and maxBytes which can be incorrect if previewBytes is shorter
for other reasons; change the computation in buildStringSearchInput to set
isTruncated based on previewBytes.size vs originalSize (e.g., isTruncated =
previewBytes.size < originalSize) so the flag reflects the actual bytes being
searched (refer to buildStringSearchInput, StringSearchInput, previewBytes,
originalSize).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c61f40ee-533b-4901-9940-b4851ecfe895
📒 Files selected for processing (2)
app/src/main/java/com/kyhsgeekcode/disassembler/ui/tabs/StringTab.ktapp/src/test/java/com/kyhsgeekcode/disassembler/ui/tabs/StringSearchInputPolicyTest.kt
Comment on lines
+52
to
+67
| internal fun buildStringSearchNotice( | ||
| input: StringSearchInput, | ||
| resultsTruncated: Boolean | ||
| ): String? { | ||
| val parts = mutableListOf<String>() | ||
| if (input.isTruncated) { | ||
| parts += "Searching strings in first ${input.bytes.size} bytes of ${input.originalSize} bytes" | ||
| } | ||
| if (resultsTruncated) { | ||
| parts += "Showing first $MAX_RENDERED_STRING_RESULTS results." | ||
| } | ||
| return when { | ||
| parts.isEmpty() -> null | ||
| parts.size == 1 -> parts.single() | ||
| else -> parts.joinToString(". ") | ||
| } |
There was a problem hiding this comment.
Don't map every truncation path to “Showing first 5000 results.”
accumulator.isTruncated is flipped both when rows are dropped and when the render-char budget clips content, but buildStringSearchNotice() renders both cases as a row-count limit. After the 32,768-char budget is exhausted, for example, the UI can show far fewer than 5,000 rows while still claiming only the first 5,000 are shown. This shape also leaves no way to surface analyzer-side 4,096-char clipping. Split count truncation from content clipping before building the notice.
🧭 Suggested direction
class StringSearchResultAccumulator(private val maxResults: Int) {
private val _results = mutableListOf<FoundString>()
val results: List<FoundString>
get() = _results
- var isTruncated: Boolean = false
+ var resultCountTruncated: Boolean = false
+ private set
+
+ var contentClipped: Boolean = false
private set
private var renderedChars: Int = 0
fun append(result: FoundString) {
if (_results.size >= maxResults) {
- isTruncated = true
+ resultCountTruncated = true
return
}
val remainingChars = MAX_RENDERED_STRING_TOTAL_CHARS - renderedChars
if (remainingChars <= 0) {
- isTruncated = true
+ contentClipped = true
return
}
val (displayResult, wasClipped) = clipFoundStringForRendering(
result,
minOf(MAX_RENDERED_STRING_CHARS, remainingChars)
)
if (wasClipped) {
- isTruncated = true
+ contentClipped = true
}
_results.add(displayResult)
renderedChars += displayResult.string.length
}
}
internal fun buildStringSearchNotice(
input: StringSearchInput,
- resultsTruncated: Boolean
+ resultCountTruncated: Boolean,
+ contentClipped: Boolean
): String? {
val parts = mutableListOf<String>()
if (input.isTruncated) {
parts += "Searching strings in first ${input.bytes.size} bytes of ${input.originalSize} bytes"
}
- if (resultsTruncated) {
+ if (resultCountTruncated) {
parts += "Showing first $MAX_RENDERED_STRING_RESULTS results."
}
+ if (contentClipped) {
+ parts += "Some matches are shortened for display."
+ }
return when {
parts.isEmpty() -> null
parts.size == 1 -> parts.single()
else -> parts.joinToString(". ")
}
}
- _notice.value = buildStringSearchNotice(input, accumulator.isTruncated)
+ _notice.value = buildStringSearchNotice(
+ input = input,
+ resultCountTruncated = accumulator.resultCountTruncated,
+ contentClipped = accumulator.contentClipped
+ )Also applies to: 99-117, 151-152
🤖 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/StringTab.kt` around
lines 52 - 67, buildStringSearchNotice currently conflates two different
truncation causes (row-count truncation and content clipping) because
input.isTruncated is overloaded; change the function to accept two distinct
booleans (e.g., countTruncated: Boolean, contentClipped: Boolean) or otherwise
distinguish input.isTruncated into those two signals at the caller sites (places
around the previous comments ~99-117 and ~151-152), then build separate notice
parts: when countTruncated is true append the existing "Showing first
$MAX_RENDERED_STRING_RESULTS results." message, and when contentClipped is true
append a message indicating rendering/content clipping (e.g., "Some results were
truncated by the render-character budget" and, if available, include analyzer
clipping like "analyzer truncated to 4096 chars"); update callers of
buildStringSearchNotice and any uses of input.isTruncated so the UI accurately
reflects count vs content clipping.
Owner
Author
|
Added another TDD slice for the large-file cluster.\n\nThis commit resets workspace state when a project is opened/imported:\n- opened tabs collapse back to a single Overview tab\n- current tab index resets to 0\n- ProjectDataStorage clears cached file content\n\nThat keeps stale large tab state and cached bytes from surviving across project switches, which is directly relevant to the remaining / memory-pressure work. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
app/src/test/java/com/kyhsgeekcode/disassembler/viewmodel/IncomingSelectionTest.kt (1)
10-18: Exercise theIntentoverload in this suite.Both tests call the parameter overload directly, so a regression in the real extraction path (
EXTRA_*keys, custom"uri", orEXTRA_STREAM) would still leave this file green. The second case also doesn't actually pass a legacy serializable extra, so its name currently overstates what it verifies.Possible test adjustment
+import android.content.Intent +import android.net.Uri import org.junit.jupiter.api.Assertions.assertEquals import org.junit.jupiter.api.Assertions.assertNull import org.junit.jupiter.api.Test @@ - fun `resolveIncomingSelection ignores legacy serializable fileItem extras`() { + fun `resolveIncomingSelection returns null when no supported selection extras are present`() { assertNull( resolveIncomingSelection( openAsProject = false, filePath = null, @@ ) ) } + + `@Test` + fun `resolveIncomingSelection reads uri from intent carrier`() { + val uri = Uri.parse("content://example/sample.apk") + + val selection = resolveIncomingSelection( + Intent().apply { putExtra(Intent.EXTRA_STREAM, uri) } + ) + + require(selection is IncomingSelection.UriSelection) + assertEquals(uri, selection.uri) + }Also applies to: 29-38
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/test/java/com/kyhsgeekcode/disassembler/viewmodel/IncomingSelectionTest.kt` around lines 10 - 18, The tests in IncomingSelectionTest.kt only call the parameter overload of resolveIncomingSelection, so update the two tests to exercise the Intent overload: construct an Intent, set the same values via intent.putExtra(EXTRA_OPEN_AS_PROJECT, true), putExtra(EXTRA_FILE_PATH, "/tmp/sample.apk"), putExtra(EXTRA_NATIVE_FILE_PATH, "/tmp/lib"), putExtra(EXTRA_PROJECT_TYPE, "APK"), include a real Uri for EXTRA_STREAM when testing extraStreamUri, and for the second case include the legacy Serializable extra (the same key the production path expects) instead of relying on the parameter call; then invoke resolveIncomingSelection(intent) and assert the same outcomes. Ensure you reference the Intent-based overload of resolveIncomingSelection and the EXTRA_* keys used by the production extraction.
🤖 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/viewmodel/MainViewModel.kt`:
- Around line 520-529: The resolveIncomingSelection(intent: Intent) path
currently only reads uri via intent.parcelableExtraCompat("uri") and
Intent.EXTRA_STREAM, so add a fallback to intent.data when building the call: in
resolveIncomingSelection(intent: Intent) replace the uri argument with
intent.data ?: intent.parcelableExtraCompat("uri") (or equivalent) so URIs
delivered via intent.data (as handled by MainActivity.extractIncomingUri() and
StartActivityForResult in MainTab) are preserved; keep the extraStream logic
untouched and ensure the resolveIncomingSelection(openAsProject, filePath,
nativeFilePath, projectType, uri, extraStreamUri, displayName) call uses that
fallback.
---
Nitpick comments:
In
`@app/src/test/java/com/kyhsgeekcode/disassembler/viewmodel/IncomingSelectionTest.kt`:
- Around line 10-18: The tests in IncomingSelectionTest.kt only call the
parameter overload of resolveIncomingSelection, so update the two tests to
exercise the Intent overload: construct an Intent, set the same values via
intent.putExtra(EXTRA_OPEN_AS_PROJECT, true), putExtra(EXTRA_FILE_PATH,
"/tmp/sample.apk"), putExtra(EXTRA_NATIVE_FILE_PATH, "/tmp/lib"),
putExtra(EXTRA_PROJECT_TYPE, "APK"), include a real Uri for EXTRA_STREAM when
testing extraStreamUri, and for the second case include the legacy Serializable
extra (the same key the production path expects) instead of relying on the
parameter call; then invoke resolveIncomingSelection(intent) and assert the same
outcomes. Ensure you reference the Intent-based overload of
resolveIncomingSelection and the EXTRA_* keys used by the production extraction.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: bfe29cd0-10d7-439b-a5b7-bdc87bc92e85
📒 Files selected for processing (2)
app/src/main/java/com/kyhsgeekcode/disassembler/viewmodel/MainViewModel.ktapp/src/test/java/com/kyhsgeekcode/disassembler/viewmodel/IncomingSelectionTest.kt
Comment on lines
+520
to
+529
| internal fun resolveIncomingSelection(intent: Intent): IncomingSelection? { | ||
| return resolveIncomingSelection( | ||
| openAsProject = intent.getBooleanExtra("openProject", false), | ||
| filePath = intent.getStringExtra(NewFileChooserActivity.EXTRA_FILE_PATH), | ||
| nativeFilePath = intent.getStringExtra(NewFileChooserActivity.EXTRA_NATIVE_FILE_PATH), | ||
| projectType = intent.getStringExtra(NewFileChooserActivity.EXTRA_PROJECT_TYPE), | ||
| uri = intent.parcelableExtraCompat("uri"), | ||
| extraStreamUri = intent.extras.parcelableExtraCompat(Intent.EXTRA_STREAM), | ||
| displayName = intent.getStringExtra("displayName") | ||
| ) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -eu
printf '\n== onSelectIntent call sites ==\n'
rg -n -C2 '\bonSelectIntent\s*\(' --type=kt
printf '\n== Producers using custom "uri" extra ==\n'
rg -n -C2 'putExtra\(\s*"uri"\s*,' --type=kt
printf '\n== Producers using intent.data / setData / setDataAndType ==\n'
rg -n -C2 '(\.data\s*=|setDataAndType\s*\(|setData\s*\()' --type=kt
printf '\n== Producers using EXTRA_STREAM ==\n'
rg -n -C2 'EXTRA_STREAM' --type=ktRepository: yhs0602/Android-Disassembler
Length of output: 148
🏁 Script executed:
#!/bin/bash
set -eu
printf '\n== onSelectIntent call sites ==\n'
rg -n -C2 'onSelectIntent\s*\(' --type kotlin
printf '\n== Producers using custom "uri" extra ==\n'
rg -n -C2 'putExtra\(\s*"uri"\s*,' --type kotlin
printf '\n== Producers using intent.data / setData / setDataAndType ==\n'
rg -n -C2 '\.data\s*=|setDataAndType|setData\(' --type kotlin
printf '\n== Producers using EXTRA_STREAM ==\n'
rg -n -C2 'EXTRA_STREAM' --type kotlinRepository: yhs0602/Android-Disassembler
Length of output: 9203
🏁 Script executed:
#!/bin/bash
# Get context around the MainTab.kt onSelectIntent call (line 63)
sed -n '50,75p' app/src/main/java/com/kyhsgeekcode/disassembler/ui/MainTab.kt
# Check if there are other activity result contracts
rg -n 'registerForActivityResult' --type kotlin -A3Repository: yhs0602/Android-Disassembler
Length of output: 1822
Add fallback to intent.data in resolveIncomingSelection(intent).
The resolver only checks custom "uri" extra and Intent.EXTRA_STREAM, but ignores intent.data. Evidence from MainActivity.extractIncomingUri() (lines 186–192) shows this app explicitly handles intent.data as a URI source, yet resolveIncomingSelection() does not. When MainTab invokes activity results via StartActivityForResult() and passes the intent directly (line 63), any URI delivered via intent.data will be dropped silently.
Add intent.data ?: intent.parcelableExtraCompat("uri") to restore the fallback:
Suggested fix
internal fun resolveIncomingSelection(intent: Intent): IncomingSelection? {
return resolveIncomingSelection(
openAsProject = intent.getBooleanExtra("openProject", false),
filePath = intent.getStringExtra(NewFileChooserActivity.EXTRA_FILE_PATH),
nativeFilePath = intent.getStringExtra(NewFileChooserActivity.EXTRA_NATIVE_FILE_PATH),
projectType = intent.getStringExtra(NewFileChooserActivity.EXTRA_PROJECT_TYPE),
- uri = intent.parcelableExtraCompat("uri"),
+ uri = intent.data ?: intent.parcelableExtraCompat("uri"),
extraStreamUri = intent.extras.parcelableExtraCompat(Intent.EXTRA_STREAM),
displayName = intent.getStringExtra("displayName")
)
}🤖 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/viewmodel/MainViewModel.kt`
around lines 520 - 529, The resolveIncomingSelection(intent: Intent) path
currently only reads uri via intent.parcelableExtraCompat("uri") and
Intent.EXTRA_STREAM, so add a fallback to intent.data when building the call: in
resolveIncomingSelection(intent: Intent) replace the uri argument with
intent.data ?: intent.parcelableExtraCompat("uri") (or equivalent) so URIs
delivered via intent.data (as handled by MainActivity.extractIncomingUri() and
StartActivityForResult in MainTab) are preserved; keep the extraStream logic
untouched and ensure the resolveIncomingSelection(openAsProject, filePath,
nativeFilePath, projectType, uri, extraStreamUri, displayName) call uses that
fallback.
There was a problem hiding this comment.
♻️ Duplicate comments (1)
app/src/main/java/com/kyhsgeekcode/disassembler/viewmodel/MainViewModel.kt (1)
551-560:⚠️ Potential issue | 🟠 MajorAdd
intent.datafallback when resolving URI selection.At Line 557, the resolver only checks the custom
"uri"extra. URI results delivered viaIntent.datacan be silently ignored. Useintent.datafirst, then fall back to extra-based URI extraction.Suggested fix
internal fun resolveIncomingSelection(intent: Intent): IncomingSelection? { return resolveIncomingSelection( openAsProject = intent.getBooleanExtra("openProject", false), filePath = intent.getStringExtra(NewFileChooserActivity.EXTRA_FILE_PATH), nativeFilePath = intent.getStringExtra(NewFileChooserActivity.EXTRA_NATIVE_FILE_PATH), projectType = intent.getStringExtra(NewFileChooserActivity.EXTRA_PROJECT_TYPE), - uri = intent.parcelableExtraCompat("uri"), + uri = intent.data ?: intent.parcelableExtraCompat("uri"), extraStreamUri = intent.extras.parcelableExtraCompat(Intent.EXTRA_STREAM), displayName = intent.getStringExtra("displayName") ) }🤖 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/viewmodel/MainViewModel.kt` around lines 551 - 560, In resolveIncomingSelection (internal fun resolveIncomingSelection(intent: Intent)), include intent.data as the primary URI source when computing the uri argument: pass intent.data if non-null, otherwise fall back to intent.parcelableExtraCompat("uri") (currently used), while keeping other parameters (openAsProject, filePath, nativeFilePath, projectType, extraStreamUri, displayName) unchanged; update the uri argument expression to prefer intent.data first so URI results delivered via Intent.data are not ignored.
🧹 Nitpick comments (1)
app/src/test/java/com/kyhsgeekcode/disassembler/viewmodel/OpenedProjectWorkspaceStateTest.kt (1)
10-20: Consider adding the “already default” invariant test too.This test covers the reset path well. Adding a second case for
previousTabs == [Overview]andpreviousCurrentTabIndex == 0would fully lock in both branches.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/test/java/com/kyhsgeekcode/disassembler/viewmodel/OpenedProjectWorkspaceStateTest.kt` around lines 10 - 20, Add a second assertion case to verify the "already default" invariant: call openedProjectWorkspaceState with previousTabs = listOf(TabData("Overview", TabKind.ProjectOverview)) and previousCurrentTabIndex = 0 (or create a separate test e.g., `openedProjectWorkspaceState leaves overview as-is`) and assert that state.currentTabIndex == 0 and state.openedTabs == listOf(TabData("Overview", TabKind.ProjectOverview)); reference the existing test setup and the openedProjectWorkspaceState helper, TabData and TabKind symbols to locate where to add this check.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@app/src/main/java/com/kyhsgeekcode/disassembler/viewmodel/MainViewModel.kt`:
- Around line 551-560: In resolveIncomingSelection (internal fun
resolveIncomingSelection(intent: Intent)), include intent.data as the primary
URI source when computing the uri argument: pass intent.data if non-null,
otherwise fall back to intent.parcelableExtraCompat("uri") (currently used),
while keeping other parameters (openAsProject, filePath, nativeFilePath,
projectType, extraStreamUri, displayName) unchanged; update the uri argument
expression to prefer intent.data first so URI results delivered via Intent.data
are not ignored.
---
Nitpick comments:
In
`@app/src/test/java/com/kyhsgeekcode/disassembler/viewmodel/OpenedProjectWorkspaceStateTest.kt`:
- Around line 10-20: Add a second assertion case to verify the "already default"
invariant: call openedProjectWorkspaceState with previousTabs =
listOf(TabData("Overview", TabKind.ProjectOverview)) and previousCurrentTabIndex
= 0 (or create a separate test e.g., `openedProjectWorkspaceState leaves
overview as-is`) and assert that state.currentTabIndex == 0 and state.openedTabs
== listOf(TabData("Overview", TabKind.ProjectOverview)); reference the existing
test setup and the openedProjectWorkspaceState helper, TabData and TabKind
symbols to locate where to add this check.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 36e92c8b-db17-450b-b329-db0b05ca3a5e
📒 Files selected for processing (11)
app/src/main/java/com/kyhsgeekcode/disassembler/project/ProjectDataStorage.ktapp/src/main/java/com/kyhsgeekcode/disassembler/viewmodel/MainViewModel.ktapp/src/main/res/layout/fragment_binary_disasm.xmlapp/src/main/res/layout/fragment_export_symbol.xmlapp/src/main/res/layout/fragment_import_symbol.xmlapp/src/main/res/layout/fragment_log.xmlapp/src/main/res/layout/fragment_string.xmlapp/src/test/java/com/kyhsgeekcode/disassembler/project/ProjectDataStorageCachePolicyTest.ktapp/src/test/java/com/kyhsgeekcode/disassembler/viewmodel/OpenedProjectWorkspaceStateTest.ktdocs/maintenance/backlog-triage.ko.mddocs/maintenance/implementation-log.ko.md
💤 Files with no reviewable changes (5)
- app/src/main/res/layout/fragment_import_symbol.xml
- app/src/main/res/layout/fragment_log.xml
- app/src/main/res/layout/fragment_export_symbol.xml
- app/src/main/res/layout/fragment_string.xml
- app/src/main/res/layout/fragment_binary_disasm.xml
Owner
Author
|
Added another instrumentation-only slice to make the suite closer to real usage.\n\nNew androidTest coverage:\n- SAF import survives activity recreate\n- Advanced import regular file works through the path and survives recreate\n- / import survives recreate\n- export cancel keeps the current project open and export remains available\n\nLocal verification on this branch:\n- Starting a Gradle Daemon, 1 incompatible and 1 stopped Daemons could not be reused, use --status for details\n- \n\nI still could not run locally because there is no attached emulator/device in this environment, so runtime execution remains gated by GitHub Actions. |
Owner
Author
|
Added another instrumentation-only slice to make the suite closer to real usage. New androidTest coverage:
Local verification on this branch:
I still could not run connectedDebugAndroidTest locally because there is no attached emulator/device in this environment, so runtime execution remains gated by GitHub Actions. |
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
StringTabIssue coverage
#219,#523, and#442Details
StringSearchResultAccumulatorVerification
JAVA_HOME=$(/usr/libexec/java_home -v 17) ANDROID_HOME=$HOME/Library/Android/sdk ./gradlew testDebugUnitTest --tests com.kyhsgeekcode.disassembler.AnalyzerStringSearchTest --tests com.kyhsgeekcode.disassembler.ui.tabs.StringSearchResultAccumulatorTestJAVA_HOME=$(/usr/libexec/java_home -v 17) ANDROID_HOME=$HOME/Library/Android/sdk ./gradlew testDebugUnitTest assembleDebugSummary by CodeRabbit
New Features
Bug Fixes
Tests
Documentation
Removed