Improve logging by placing logs in core functions#96
Conversation
tuancoltech
force-pushed
the
feat/improve_logging
branch
from
a74b0de to
322196c
Compare
tuancoltech
force-pushed
the
feat/improve_logging
branch
from
322196c to
2bb08f0
Compare
tuancoltech
force-pushed
the
feat/improve_logging
branch
from
2bb08f0 to
49ad4cb
Compare
|
@coderabbitai Do a code review. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chainI'll conduct a code review of PR #96 for you. Let me first gather information about the changes made in this pull request. 🏁 Script executed: Length of output: 29265 @tuancoltech I've reviewed PR #96 which adds logging to improve debugging capabilities across the ARK Memo application. Here's my comprehensive code review: ✅ Positive Aspects
🔍 Detailed AnalysisMedia Layer (
|
| @@ -202,6 +206,7 @@ class MainActivity : AppCompatActivity(R.layout.activity_main) { | |||
|
|
|||
| companion object { | |||
| private const val CURRENT_FRAGMENT_TAG = "current fragment tag" | |||
There was a problem hiding this comment.
| private const val CURRENT_FRAGMENT_TAG = "current fragment tag" | |
| private const val CURRENT_FRAGMENT_TAG = "CurrentFragment" |
There was a problem hiding this comment.
@shubertm This change wasn't made on this PR, and unrelated.
However, I still updated the value to remove whitespaces.
| note: Note, | ||
| parentResId: ResourceId? = null, | ||
| ) { | ||
| Log.d(tag, "add note: $note") |
There was a problem hiding this comment.
Better to print note resource data e.g; id or name
tuancoltech
force-pushed
the
feat/improve_logging
branch
from
49ad4cb to
a39de21
Compare
WalkthroughThis update introduces debug and error logging statements throughout various classes in the project, primarily for tracing method invocations, key events, and error occurrences. Logging tags are standardized using constants or companion objects. No changes are made to the core logic, control flow, or error handling of the affected classes. Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~8 minutes Poem
Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ 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). (1)
✨ Finishing Touches
🧪 Generate unit tests
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![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
🔭 Outside diff range comments (4)
app/src/main/java/dev/arkbuilders/arkmemo/repo/text/TextNotesRepo.kt (1)
63-70: Logging full note contents exposes PII
note.textcan contain sensitive user data. Avoid dumping the entire object.-Log.d(TEXT_REPO, "write note: $note") +Log.d(TEXT_REPO, "write note title='${note.title}' size=${note.text.length}")Do the same wherever note bodies are logged.
app/src/main/java/dev/arkbuilders/arkmemo/media/ArkMediaPlayerImpl.kt (1)
57-78: High-frequency methods should avoid unconditional logging
play(),pause(), and particularlyseekTo()may fire dozens of times per second while a user scrubs. RepeatedLog.dcalls can produce noticeable GC pressure on low-end devices. Either:
- wrap with
if (BuildConfig.DEBUG)or- throttle logging (e.g. only once every N ms).
app/src/main/java/dev/arkbuilders/arkmemo/ui/viewmodels/NotesViewModel.kt (1)
174-181: Potential PII leakage in logs
Logging note titles and resource IDs (add note with title: …) may inadvertently expose user data in release builds. Suggest downgrading to verbose level guarded byBuildConfig.DEBUGor logging only the ID.app/src/main/java/dev/arkbuilders/arkmemo/repo/NotesRepoHelper.kt (1)
61-77: Persist-properties log may expose note titles
Same privacy concern as inNotesViewModel. Title could contain sensitive data; consider obfuscating or removing in release.
♻️ Duplicate comments (1)
app/src/main/java/dev/arkbuilders/arkmemo/ui/viewmodels/GraphicNotesViewModel.kt (1)
26-27: Align TAG declaration with project-wide patternOther view-models now use a companion-object
TAG. Adopt the same to avoid divergent style.- private val tag = "GraphicNotesViewModel" + companion object { + private const val TAG = "GraphicNotesViewModel" + }
🧹 Nitpick comments (16)
app/src/main/java/dev/arkbuilders/arkmemo/ui/views/WaveView.kt (1)
51-51: Guard high-frequency debug log behind a verbosity check
computeWaveis invoked for every visual frame; unconditionalLog.dwill allocate the interpolated string each call, impacting UI thread performance.
Wrap the call inLog.isLoggable(TAG, Log.DEBUG)(orif (BuildConfig.DEBUG)) to avoid work when debug logs are disabled.- Log.d(TAG, "computeWave amplitude: $amplitude height: $height") + if (Log.isLoggable(TAG, Log.DEBUG)) { + Log.d(TAG, "computeWave amplitude: $amplitude height: $height") + }app/src/main/java/dev/arkbuilders/arkmemo/repo/voices/VoiceNotesRepo.kt (1)
61-62: Wrap low-value debug logs withBuildConfig.DEBUGand enrich the message
BothwriteandreadStorageare on critical I/O paths and may be invoked many times; in release builds the extra string allocations still happen even if the Logcat is disabled by policy. Guarding them avoids that overhead and lets you keep the noise out of production logs. While touching the lines, include some context (note id / list size) to make the entry actually useful during debugging.-import android.util.Log +import android.util.Log +import dev.arkbuilders.arkmemo.BuildConfig … - Log.d(VOICES_REPO, "write") + if (BuildConfig.DEBUG) { + Log.d(VOICES_REPO, "write title=${note.title} size=$size") + } … - Log.d(VOICES_REPO, "readStorage") + if (BuildConfig.DEBUG) { + Log.d(VOICES_REPO, "readStorage count=${root.listFiles(VOICE_EXT).size}") + }Also applies to: 104-105
app/src/main/java/dev/arkbuilders/arkmemo/ui/viewmodels/QRViewModel.kt (2)
31-33:TAGdefinition is good – add@JvmFieldfor proguard friendliness
Marking the constant@JvmFieldremoves the synthetic getter, slightly reducing method count and preventing pro-guard from stripping usages.- companion object { - private const val TAG = "QRViewModel" - } + companion object { + @JvmField + internal const val TAG = "QRViewModel" + }
39-40: Guard debug logs & supply context
Same rationale as in the repo layer: avoid work in production and make the message self-describing.- Log.d(TAG, "generateQRCode") + if (BuildConfig.DEBUG) Log.d(TAG, "generateQRCode text.length=${text.length}") … - Log.d(TAG, "saveQRCodeImage") + if (BuildConfig.DEBUG) Log.d(TAG, "saveQRCodeImage text.length=${text.length}")Also applies to: 56-57
app/src/main/java/dev/arkbuilders/arkmemo/ui/fragments/NotesFragment.kt (1)
318-320: Release-safe logging & better signal
onNotesLoadedruns on every screen open and after every CRUD; unguarded debug logging will appear in Play-store builds unless pro-guard strips it. At minimum:-import android.util.Log +import android.util.Log +import dev.arkbuilders.arkmemo.BuildConfig … - Log.d(TAG, "onNotesLoaded notes.size: ${notes.size}") + if (BuildConfig.DEBUG) Log.d(TAG, "onNotesLoaded size=${notes.size}")app/src/main/java/dev/arkbuilders/arkmemo/ui/fragments/ArkMediaPlayerFragment.kt (1)
103-104:TAGlength exceeds 23 chars & log should be debug-guarded
Pre-API-26 devices truncate tags longer than 23 chars. Consider shortening toArkMediaFragment. Also guard the call:- Log.d(TAG, "handleSideEffect: $effect") + if (BuildConfig.DEBUG) Log.d(TAG, "sideEffect=$effect")app/src/main/java/dev/arkbuilders/arkmemo/ui/fragments/ArkRecorderFragment.kt (1)
318-320: Same logging concerns as other files
Guard the log and keep the tag short & consistent.- Log.d(TAG, "handlePlaySideEffect: $effect") + if (BuildConfig.DEBUG) Log.d(TAG, "playSideEffect=$effect")app/src/main/java/dev/arkbuilders/arkmemo/media/ArkAudioRecorderImpl.kt (2)
17-18: Prefer a companion-objectTAGconstant for consistencyMost other classes in this PR switched to
companion object { private const val TAG = "..." }``` Using a `val tag` instance property breaks that convention and creates an extra field per object. ```diff - private val tag = "ArkAudioRecorderImpl" + companion object { + private const val TAG = "ArkAudioRecorderImpl" + }
39-55: Guard debug logs withisLoggable/BuildConfig.DEBUG
start(),pause(),resume()andreset()are user-driven, butmaxAmplitude()may be polled many times per second.
Unconditionally emittingLog.d()can spam logcat in production builds.-Log.d(TAG, "start") +if (Log.isLoggable(TAG, Log.DEBUG)) Log.d(TAG, "start")Apply the same pattern to the other debug logs in this class.
Also applies to: 75-76, 84-85
app/src/main/java/dev/arkbuilders/arkmemo/ui/viewmodels/ArkRecorderViewModel.kt (1)
3-4: Minor consistency nitFollow the same companion-object
TAGformat suggested elsewhere to keep the codebase uniform.-companion object { - private const val TAG = "ArkRecorderViewModel" -} +companion object { + private const val TAG = "ArkRecorderViewModel" +}Also applies to: 42-45
app/src/main/java/dev/arkbuilders/arkmemo/media/ArkMediaPlayerImpl.kt (1)
27-33: Consider guarding verbose logs withBuildConfig.DEBUG
init()can be called repeatedly during screen rotation or playlist changes, so these debug lines will end up in release builds and potentially pollute logcat.-Log.d(TAG, "init") +if (BuildConfig.DEBUG) Log.d(TAG, "init")Apply the same guard to every other debug call in this class.
Also applies to: 50-52
app/src/main/java/dev/arkbuilders/arkmemo/ui/viewmodels/NotesViewModel.kt (2)
52-68: Release-build spam risk
initandreadAllNotesare executed on every app launch; consider wrapping theseLog.dcalls withBuildConfig.DEBUGto keep production logs clean.
117-118: Log message uses enumname; prefer full object for clarity
result.namecollapses both success branches (SUCCESS_NEW,SUCCESS_UPDATED) to very similar strings. Using the enum itself or a descriptive sentence improves greppability.app/src/main/java/dev/arkbuilders/arkmemo/repo/graphics/GraphicNotesRepo.kt (2)
83-89: Path names in logs can leak user directories
initial resource name is ${tempPath.name}is fine, but elsewhere full paths (path) are logged. Consider trimming tofileNameonly or guarding withBuildConfig.DEBUG.
118-126: Guard repository read spam
readStorage()is invoked every refresh; theLog.dcall will fire for every action. Recommendif (BuildConfig.DEBUG)to avoid noisy release logs.app/src/main/java/dev/arkbuilders/arkmemo/repo/NotesRepoHelper.kt (1)
50-53: Consider movingLog.d(TAG, "init")behind debug flag
Minor, but aligns with other suggestions across modules.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
app/src/main/java/dev/arkbuilders/arkmemo/media/ArkAudioRecorderImpl.kt(3 hunks)app/src/main/java/dev/arkbuilders/arkmemo/media/ArkMediaPlayerImpl.kt(3 hunks)app/src/main/java/dev/arkbuilders/arkmemo/repo/NotesRepoHelper.kt(6 hunks)app/src/main/java/dev/arkbuilders/arkmemo/repo/graphics/GraphicNotesRepo.kt(4 hunks)app/src/main/java/dev/arkbuilders/arkmemo/repo/text/TextNotesRepo.kt(3 hunks)app/src/main/java/dev/arkbuilders/arkmemo/repo/voices/VoiceNotesRepo.kt(2 hunks)app/src/main/java/dev/arkbuilders/arkmemo/ui/activities/MainActivity.kt(5 hunks)app/src/main/java/dev/arkbuilders/arkmemo/ui/fragments/ArkMediaPlayerFragment.kt(2 hunks)app/src/main/java/dev/arkbuilders/arkmemo/ui/fragments/ArkRecorderFragment.kt(2 hunks)app/src/main/java/dev/arkbuilders/arkmemo/ui/fragments/NotesFragment.kt(2 hunks)app/src/main/java/dev/arkbuilders/arkmemo/ui/viewmodels/ArkMediaPlayerViewModel.kt(5 hunks)app/src/main/java/dev/arkbuilders/arkmemo/ui/viewmodels/ArkRecorderViewModel.kt(7 hunks)app/src/main/java/dev/arkbuilders/arkmemo/ui/viewmodels/GraphicNotesViewModel.kt(3 hunks)app/src/main/java/dev/arkbuilders/arkmemo/ui/viewmodels/NotesViewModel.kt(6 hunks)app/src/main/java/dev/arkbuilders/arkmemo/ui/viewmodels/QRViewModel.kt(3 hunks)app/src/main/java/dev/arkbuilders/arkmemo/ui/views/WaveView.kt(3 hunks)
🧠 Learnings (12)
app/src/main/java/dev/arkbuilders/arkmemo/ui/fragments/ArkMediaPlayerFragment.kt (2)
Learnt from: tuancoltech
PR: ARK-Builders/ARK-Memo#0
File: :0-0
Timestamp: 2025-07-18T05:40:19.332Z
Learning: In the ARK-Memo Android app, the notesViewModel.init {} callback properly handles threading by dispatching operations to worker threads, preventing main thread blocking during view restoration.
Learnt from: tuancoltech
PR: ARK-Builders/ARK-Memo#0
File: :0-0
Timestamp: 2025-07-18T05:33:52.353Z
Learning: In the ARK-Memo Android app, TextNotesRepo.findNote() intentionally returns null because text notes don't need special handling during view restoration. This is a design decision to ignore text notes when restoring view state, as users can easily continue editing text notes without complex state recovery mechanisms.
app/src/main/java/dev/arkbuilders/arkmemo/ui/fragments/NotesFragment.kt (2)
Learnt from: tuancoltech
PR: ARK-Builders/ARK-Memo#0
File: :0-0
Timestamp: 2025-07-18T05:33:52.353Z
Learning: In the ARK-Memo Android app, TextNotesRepo.findNote() intentionally returns null because text notes don't need special handling during view restoration. This is a design decision to ignore text notes when restoring view state, as users can easily continue editing text notes without complex state recovery mechanisms.
Learnt from: tuancoltech
PR: ARK-Builders/ARK-Memo#0
File: :0-0
Timestamp: 2025-07-18T05:40:19.332Z
Learning: In the ARK-Memo Android app, the notesViewModel.init {} callback properly handles threading by dispatching operations to worker threads, preventing main thread blocking during view restoration.
app/src/main/java/dev/arkbuilders/arkmemo/repo/text/TextNotesRepo.kt (2)
Learnt from: tuancoltech
PR: ARK-Builders/ARK-Memo#0
File: :0-0
Timestamp: 2025-07-18T05:33:52.353Z
Learning: In the ARK-Memo Android app, TextNotesRepo.findNote() intentionally returns null because text notes don't need special handling during view restoration. This is a design decision to ignore text notes when restoring view state, as users can easily continue editing text notes without complex state recovery mechanisms.
Learnt from: tuancoltech
PR: ARK-Builders/ARK-Memo#0
File: :0-0
Timestamp: 2025-07-18T05:40:19.332Z
Learning: In the ARK-Memo Android app, the notesViewModel.init {} callback properly handles threading by dispatching operations to worker threads, preventing main thread blocking during view restoration.
app/src/main/java/dev/arkbuilders/arkmemo/repo/voices/VoiceNotesRepo.kt (2)
Learnt from: tuancoltech
PR: ARK-Builders/ARK-Memo#0
File: :0-0
Timestamp: 2025-07-18T05:33:52.353Z
Learning: In the ARK-Memo Android app, TextNotesRepo.findNote() intentionally returns null because text notes don't need special handling during view restoration. This is a design decision to ignore text notes when restoring view state, as users can easily continue editing text notes without complex state recovery mechanisms.
Learnt from: tuancoltech
PR: ARK-Builders/ARK-Memo#0
File: :0-0
Timestamp: 2025-07-18T05:40:19.332Z
Learning: In the ARK-Memo Android app, the notesViewModel.init {} callback properly handles threading by dispatching operations to worker threads, preventing main thread blocking during view restoration.
app/src/main/java/dev/arkbuilders/arkmemo/ui/viewmodels/ArkMediaPlayerViewModel.kt (2)
Learnt from: tuancoltech
PR: ARK-Builders/ARK-Memo#0
File: :0-0
Timestamp: 2025-07-18T05:40:19.332Z
Learning: In the ARK-Memo Android app, the notesViewModel.init {} callback properly handles threading by dispatching operations to worker threads, preventing main thread blocking during view restoration.
Learnt from: tuancoltech
PR: ARK-Builders/ARK-Memo#0
File: :0-0
Timestamp: 2025-07-18T05:33:52.353Z
Learning: In the ARK-Memo Android app, TextNotesRepo.findNote() intentionally returns null because text notes don't need special handling during view restoration. This is a design decision to ignore text notes when restoring view state, as users can easily continue editing text notes without complex state recovery mechanisms.
app/src/main/java/dev/arkbuilders/arkmemo/ui/viewmodels/QRViewModel.kt (1)
Learnt from: tuancoltech
PR: ARK-Builders/ARK-Memo#0
File: :0-0
Timestamp: 2025-07-18T05:40:19.332Z
Learning: In the ARK-Memo Android app, the notesViewModel.init {} callback properly handles threading by dispatching operations to worker threads, preventing main thread blocking during view restoration.
app/src/main/java/dev/arkbuilders/arkmemo/ui/viewmodels/GraphicNotesViewModel.kt (2)
Learnt from: tuancoltech
PR: ARK-Builders/ARK-Memo#0
File: :0-0
Timestamp: 2025-07-18T05:40:19.332Z
Learning: In the ARK-Memo Android app, the notesViewModel.init {} callback properly handles threading by dispatching operations to worker threads, preventing main thread blocking during view restoration.
Learnt from: tuancoltech
PR: ARK-Builders/ARK-Memo#0
File: :0-0
Timestamp: 2025-07-18T05:33:52.353Z
Learning: In the ARK-Memo Android app, TextNotesRepo.findNote() intentionally returns null because text notes don't need special handling during view restoration. This is a design decision to ignore text notes when restoring view state, as users can easily continue editing text notes without complex state recovery mechanisms.
app/src/main/java/dev/arkbuilders/arkmemo/ui/viewmodels/NotesViewModel.kt (2)
Learnt from: tuancoltech
PR: ARK-Builders/ARK-Memo#0
File: :0-0
Timestamp: 2025-07-18T05:40:19.332Z
Learning: In the ARK-Memo Android app, the notesViewModel.init {} callback properly handles threading by dispatching operations to worker threads, preventing main thread blocking during view restoration.
Learnt from: tuancoltech
PR: ARK-Builders/ARK-Memo#0
File: :0-0
Timestamp: 2025-07-18T05:33:52.353Z
Learning: In the ARK-Memo Android app, TextNotesRepo.findNote() intentionally returns null because text notes don't need special handling during view restoration. This is a design decision to ignore text notes when restoring view state, as users can easily continue editing text notes without complex state recovery mechanisms.
app/src/main/java/dev/arkbuilders/arkmemo/repo/graphics/GraphicNotesRepo.kt (2)
Learnt from: tuancoltech
PR: ARK-Builders/ARK-Memo#0
File: :0-0
Timestamp: 2025-07-18T05:33:52.353Z
Learning: In the ARK-Memo Android app, TextNotesRepo.findNote() intentionally returns null because text notes don't need special handling during view restoration. This is a design decision to ignore text notes when restoring view state, as users can easily continue editing text notes without complex state recovery mechanisms.
Learnt from: tuancoltech
PR: ARK-Builders/ARK-Memo#0
File: :0-0
Timestamp: 2025-07-18T05:40:19.332Z
Learning: In the ARK-Memo Android app, the notesViewModel.init {} callback properly handles threading by dispatching operations to worker threads, preventing main thread blocking during view restoration.
app/src/main/java/dev/arkbuilders/arkmemo/ui/activities/MainActivity.kt (2)
Learnt from: tuancoltech
PR: ARK-Builders/ARK-Memo#0
File: :0-0
Timestamp: 2025-07-18T05:40:19.332Z
Learning: In the ARK-Memo Android app, the notesViewModel.init {} callback properly handles threading by dispatching operations to worker threads, preventing main thread blocking during view restoration.
Learnt from: tuancoltech
PR: ARK-Builders/ARK-Memo#0
File: :0-0
Timestamp: 2025-07-18T05:33:52.353Z
Learning: In the ARK-Memo Android app, TextNotesRepo.findNote() intentionally returns null because text notes don't need special handling during view restoration. This is a design decision to ignore text notes when restoring view state, as users can easily continue editing text notes without complex state recovery mechanisms.
app/src/main/java/dev/arkbuilders/arkmemo/repo/NotesRepoHelper.kt (2)
Learnt from: tuancoltech
PR: ARK-Builders/ARK-Memo#0
File: :0-0
Timestamp: 2025-07-18T05:33:52.353Z
Learning: In the ARK-Memo Android app, TextNotesRepo.findNote() intentionally returns null because text notes don't need special handling during view restoration. This is a design decision to ignore text notes when restoring view state, as users can easily continue editing text notes without complex state recovery mechanisms.
Learnt from: tuancoltech
PR: ARK-Builders/ARK-Memo#0
File: :0-0
Timestamp: 2025-07-18T05:40:19.332Z
Learning: In the ARK-Memo Android app, the notesViewModel.init {} callback properly handles threading by dispatching operations to worker threads, preventing main thread blocking during view restoration.
app/src/main/java/dev/arkbuilders/arkmemo/ui/viewmodels/ArkRecorderViewModel.kt (2)
Learnt from: tuancoltech
PR: ARK-Builders/ARK-Memo#0
File: :0-0
Timestamp: 2025-07-18T05:40:19.332Z
Learning: In the ARK-Memo Android app, the notesViewModel.init {} callback properly handles threading by dispatching operations to worker threads, preventing main thread blocking during view restoration.
Learnt from: tuancoltech
PR: ARK-Builders/ARK-Memo#0
File: :0-0
Timestamp: 2025-07-18T05:33:52.353Z
Learning: In the ARK-Memo Android app, TextNotesRepo.findNote() intentionally returns null because text notes don't need special handling during view restoration. This is a design decision to ignore text notes when restoring view state, as users can easily continue editing text notes without complex state recovery mechanisms.
🧰 Additional context used
🧠 Learnings (12)
app/src/main/java/dev/arkbuilders/arkmemo/ui/fragments/ArkMediaPlayerFragment.kt (2)
Learnt from: tuancoltech
PR: ARK-Builders/ARK-Memo#0
File: :0-0
Timestamp: 2025-07-18T05:40:19.332Z
Learning: In the ARK-Memo Android app, the notesViewModel.init {} callback properly handles threading by dispatching operations to worker threads, preventing main thread blocking during view restoration.
Learnt from: tuancoltech
PR: ARK-Builders/ARK-Memo#0
File: :0-0
Timestamp: 2025-07-18T05:33:52.353Z
Learning: In the ARK-Memo Android app, TextNotesRepo.findNote() intentionally returns null because text notes don't need special handling during view restoration. This is a design decision to ignore text notes when restoring view state, as users can easily continue editing text notes without complex state recovery mechanisms.
app/src/main/java/dev/arkbuilders/arkmemo/ui/fragments/NotesFragment.kt (2)
Learnt from: tuancoltech
PR: ARK-Builders/ARK-Memo#0
File: :0-0
Timestamp: 2025-07-18T05:33:52.353Z
Learning: In the ARK-Memo Android app, TextNotesRepo.findNote() intentionally returns null because text notes don't need special handling during view restoration. This is a design decision to ignore text notes when restoring view state, as users can easily continue editing text notes without complex state recovery mechanisms.
Learnt from: tuancoltech
PR: ARK-Builders/ARK-Memo#0
File: :0-0
Timestamp: 2025-07-18T05:40:19.332Z
Learning: In the ARK-Memo Android app, the notesViewModel.init {} callback properly handles threading by dispatching operations to worker threads, preventing main thread blocking during view restoration.
app/src/main/java/dev/arkbuilders/arkmemo/repo/text/TextNotesRepo.kt (2)
Learnt from: tuancoltech
PR: ARK-Builders/ARK-Memo#0
File: :0-0
Timestamp: 2025-07-18T05:33:52.353Z
Learning: In the ARK-Memo Android app, TextNotesRepo.findNote() intentionally returns null because text notes don't need special handling during view restoration. This is a design decision to ignore text notes when restoring view state, as users can easily continue editing text notes without complex state recovery mechanisms.
Learnt from: tuancoltech
PR: ARK-Builders/ARK-Memo#0
File: :0-0
Timestamp: 2025-07-18T05:40:19.332Z
Learning: In the ARK-Memo Android app, the notesViewModel.init {} callback properly handles threading by dispatching operations to worker threads, preventing main thread blocking during view restoration.
app/src/main/java/dev/arkbuilders/arkmemo/repo/voices/VoiceNotesRepo.kt (2)
Learnt from: tuancoltech
PR: ARK-Builders/ARK-Memo#0
File: :0-0
Timestamp: 2025-07-18T05:33:52.353Z
Learning: In the ARK-Memo Android app, TextNotesRepo.findNote() intentionally returns null because text notes don't need special handling during view restoration. This is a design decision to ignore text notes when restoring view state, as users can easily continue editing text notes without complex state recovery mechanisms.
Learnt from: tuancoltech
PR: ARK-Builders/ARK-Memo#0
File: :0-0
Timestamp: 2025-07-18T05:40:19.332Z
Learning: In the ARK-Memo Android app, the notesViewModel.init {} callback properly handles threading by dispatching operations to worker threads, preventing main thread blocking during view restoration.
app/src/main/java/dev/arkbuilders/arkmemo/ui/viewmodels/ArkMediaPlayerViewModel.kt (2)
Learnt from: tuancoltech
PR: ARK-Builders/ARK-Memo#0
File: :0-0
Timestamp: 2025-07-18T05:40:19.332Z
Learning: In the ARK-Memo Android app, the notesViewModel.init {} callback properly handles threading by dispatching operations to worker threads, preventing main thread blocking during view restoration.
Learnt from: tuancoltech
PR: ARK-Builders/ARK-Memo#0
File: :0-0
Timestamp: 2025-07-18T05:33:52.353Z
Learning: In the ARK-Memo Android app, TextNotesRepo.findNote() intentionally returns null because text notes don't need special handling during view restoration. This is a design decision to ignore text notes when restoring view state, as users can easily continue editing text notes without complex state recovery mechanisms.
app/src/main/java/dev/arkbuilders/arkmemo/ui/viewmodels/QRViewModel.kt (1)
Learnt from: tuancoltech
PR: ARK-Builders/ARK-Memo#0
File: :0-0
Timestamp: 2025-07-18T05:40:19.332Z
Learning: In the ARK-Memo Android app, the notesViewModel.init {} callback properly handles threading by dispatching operations to worker threads, preventing main thread blocking during view restoration.
app/src/main/java/dev/arkbuilders/arkmemo/ui/viewmodels/GraphicNotesViewModel.kt (2)
Learnt from: tuancoltech
PR: ARK-Builders/ARK-Memo#0
File: :0-0
Timestamp: 2025-07-18T05:40:19.332Z
Learning: In the ARK-Memo Android app, the notesViewModel.init {} callback properly handles threading by dispatching operations to worker threads, preventing main thread blocking during view restoration.
Learnt from: tuancoltech
PR: ARK-Builders/ARK-Memo#0
File: :0-0
Timestamp: 2025-07-18T05:33:52.353Z
Learning: In the ARK-Memo Android app, TextNotesRepo.findNote() intentionally returns null because text notes don't need special handling during view restoration. This is a design decision to ignore text notes when restoring view state, as users can easily continue editing text notes without complex state recovery mechanisms.
app/src/main/java/dev/arkbuilders/arkmemo/ui/viewmodels/NotesViewModel.kt (2)
Learnt from: tuancoltech
PR: ARK-Builders/ARK-Memo#0
File: :0-0
Timestamp: 2025-07-18T05:40:19.332Z
Learning: In the ARK-Memo Android app, the notesViewModel.init {} callback properly handles threading by dispatching operations to worker threads, preventing main thread blocking during view restoration.
Learnt from: tuancoltech
PR: ARK-Builders/ARK-Memo#0
File: :0-0
Timestamp: 2025-07-18T05:33:52.353Z
Learning: In the ARK-Memo Android app, TextNotesRepo.findNote() intentionally returns null because text notes don't need special handling during view restoration. This is a design decision to ignore text notes when restoring view state, as users can easily continue editing text notes without complex state recovery mechanisms.
app/src/main/java/dev/arkbuilders/arkmemo/repo/graphics/GraphicNotesRepo.kt (2)
Learnt from: tuancoltech
PR: ARK-Builders/ARK-Memo#0
File: :0-0
Timestamp: 2025-07-18T05:33:52.353Z
Learning: In the ARK-Memo Android app, TextNotesRepo.findNote() intentionally returns null because text notes don't need special handling during view restoration. This is a design decision to ignore text notes when restoring view state, as users can easily continue editing text notes without complex state recovery mechanisms.
Learnt from: tuancoltech
PR: ARK-Builders/ARK-Memo#0
File: :0-0
Timestamp: 2025-07-18T05:40:19.332Z
Learning: In the ARK-Memo Android app, the notesViewModel.init {} callback properly handles threading by dispatching operations to worker threads, preventing main thread blocking during view restoration.
app/src/main/java/dev/arkbuilders/arkmemo/ui/activities/MainActivity.kt (2)
Learnt from: tuancoltech
PR: ARK-Builders/ARK-Memo#0
File: :0-0
Timestamp: 2025-07-18T05:40:19.332Z
Learning: In the ARK-Memo Android app, the notesViewModel.init {} callback properly handles threading by dispatching operations to worker threads, preventing main thread blocking during view restoration.
Learnt from: tuancoltech
PR: ARK-Builders/ARK-Memo#0
File: :0-0
Timestamp: 2025-07-18T05:33:52.353Z
Learning: In the ARK-Memo Android app, TextNotesRepo.findNote() intentionally returns null because text notes don't need special handling during view restoration. This is a design decision to ignore text notes when restoring view state, as users can easily continue editing text notes without complex state recovery mechanisms.
app/src/main/java/dev/arkbuilders/arkmemo/repo/NotesRepoHelper.kt (2)
Learnt from: tuancoltech
PR: ARK-Builders/ARK-Memo#0
File: :0-0
Timestamp: 2025-07-18T05:33:52.353Z
Learning: In the ARK-Memo Android app, TextNotesRepo.findNote() intentionally returns null because text notes don't need special handling during view restoration. This is a design decision to ignore text notes when restoring view state, as users can easily continue editing text notes without complex state recovery mechanisms.
Learnt from: tuancoltech
PR: ARK-Builders/ARK-Memo#0
File: :0-0
Timestamp: 2025-07-18T05:40:19.332Z
Learning: In the ARK-Memo Android app, the notesViewModel.init {} callback properly handles threading by dispatching operations to worker threads, preventing main thread blocking during view restoration.
app/src/main/java/dev/arkbuilders/arkmemo/ui/viewmodels/ArkRecorderViewModel.kt (2)
Learnt from: tuancoltech
PR: ARK-Builders/ARK-Memo#0
File: :0-0
Timestamp: 2025-07-18T05:40:19.332Z
Learning: In the ARK-Memo Android app, the notesViewModel.init {} callback properly handles threading by dispatching operations to worker threads, preventing main thread blocking during view restoration.
Learnt from: tuancoltech
PR: ARK-Builders/ARK-Memo#0
File: :0-0
Timestamp: 2025-07-18T05:33:52.353Z
Learning: In the ARK-Memo Android app, TextNotesRepo.findNote() intentionally returns null because text notes don't need special handling during view restoration. This is a design decision to ignore text notes when restoring view state, as users can easily continue editing text notes without complex state recovery mechanisms.
🔇 Additional comments (11)
app/src/main/java/dev/arkbuilders/arkmemo/ui/views/WaveView.kt (1)
71-71: TAG constant extracted – looks goodIntroducing a
TAGconstant satisfies the prior feedback and keeps logging consistent with the rest of the codebase. 👍app/src/main/java/dev/arkbuilders/arkmemo/ui/viewmodels/ArkMediaPlayerViewModel.kt (1)
79-80: Optional: remove one-shot debug traces after validation
setupVisualizer()andstartProgressMonitor()are invoked once per playback session; leaving these logs is probably fine. Just ensure they are compiled out of release builds via ProGuard/R8 rules (-assumenosideeffects class android.util.Log { *; }).Also applies to: 175-176
app/src/main/java/dev/arkbuilders/arkmemo/repo/text/TextNotesRepo.kt (1)
100-101: Good: added tracing & error logThe lightweight
readStorageentry trace and detailed exception log improve debuggability without exposing user data.Also applies to: 128-129
app/src/main/java/dev/arkbuilders/arkmemo/ui/viewmodels/ArkRecorderViewModel.kt (1)
59-79: Event logs look goodThe added logs fire only on explicit UI actions, so the volume is negligible and safe for production.
Also applies to: 111-113, 137-139
app/src/main/java/dev/arkbuilders/arkmemo/media/ArkMediaPlayerImpl.kt (1)
18-21: TAG constant nicely centralised
Using a companion-object constant removes the earlier string‐literal repetition – good catch.app/src/main/java/dev/arkbuilders/arkmemo/ui/viewmodels/NotesViewModel.kt (1)
47-50: TAG constant added – 👍
Consistent with the rest of the codebase.app/src/main/java/dev/arkbuilders/arkmemo/repo/graphics/GraphicNotesRepo.kt (1)
152-197: Null-log correctly handled – LGTM
Warning on missing SVG or paths is helpful and already atLog.w; no action needed.app/src/main/java/dev/arkbuilders/arkmemo/repo/NotesRepoHelper.kt (2)
37-40: TAG constant addition – good consistency
122-138: Deletion path logged – acceptable
File names are already on disk; debug line is reasonable.app/src/main/java/dev/arkbuilders/arkmemo/ui/activities/MainActivity.kt (2)
45-57: Permission-callbacks logging is fine
Lightweight, fires once.
208-210: Constant rename improves clarity
LGTM.
| supportFragmentManager.onArkPathPicked(this) { | ||
| Log.d(ACTIVITY_TAG, "onArkPathPicked path: $it") | ||
| memoPreferences.storePath(it.toString()) | ||
| showFragment(savedInstanceState) | ||
| } |
There was a problem hiding this comment.
Full storage path logged – possible privacy issue
onArkPathPicked path: $it prints the absolute directory chosen by the user. On shared devices this leaks personal folder structure. Limit to last path segment or guard with BuildConfig.DEBUG.
🤖 Prompt for AI Agents
In app/src/main/java/dev/arkbuilders/arkmemo/ui/activities/MainActivity.kt
around lines 73 to 77, the log statement prints the full absolute path chosen by
the user, which can expose sensitive directory information. Modify the log to
either only print the last segment of the path or wrap the logging statement
inside a check for BuildConfig.DEBUG to ensure it only logs in debug builds,
preventing privacy leaks on shared devices.
There was a problem hiding this comment.
@tuancoltech I think @coderabbitai is right here, let's consider this improvement
There was a problem hiding this comment.
@shubertm Thank you for supporting this privacy improvement! Here are two concrete approaches @tuancoltech could implement:
Option 1: Log only the directory name
Log.d(ACTIVITY_TAG, "onArkPathPicked path: ${File(it.toString()).name}")Option 2: Debug-only logging
if (BuildConfig.DEBUG) {
Log.d(ACTIVITY_TAG, "onArkPathPicked path: $it")
}Option 3: Both (recommended)
if (BuildConfig.DEBUG) {
Log.d(ACTIVITY_TAG, "onArkPathPicked path: ${File(it.toString()).name}")
}Option 3 provides the best privacy protection by only logging in debug builds and limiting the exposed information to just the folder name rather than the full path structure.
🐰💭
(\ /)
( ..)
o(")(")
🧠 Learnings used
Learnt from: tuancoltech
PR: ARK-Builders/ARK-Memo#0
File: :0-0
Timestamp: 2025-07-18T05:40:19.332Z
Learning: In the ARK-Memo Android app, the notesViewModel.init {} callback properly handles threading by dispatching operations to worker threads, preventing main thread blocking during view restoration.
Learnt from: tuancoltech
PR: ARK-Builders/ARK-Memo#0
File: :0-0
Timestamp: 2025-07-18T05:33:52.353Z
Learning: In the ARK-Memo Android app, TextNotesRepo.findNote() intentionally returns null because text notes don't need special handling during view restoration. This is a design decision to ignore text notes when restoring view state, as users can easily continue editing text notes without complex state recovery mechanisms.
| Log.d(TAG, "computeFftMagnitude") | ||
| if (fft == null) return 0f | ||
|
|
There was a problem hiding this comment.
High-frequency log in computeFftMagnitude() may hurt performance
computeFftMagnitude() runs on every Visualizer callback (typically ~20-30 Hz).
Even at DEBUG level this can flood logcat and trigger string allocations.
Wrap the call:
if (Log.isLoggable(TAG, Log.DEBUG)) Log.d(TAG, "computeFftMagnitude")or guard by BuildConfig.DEBUG.
🤖 Prompt for AI Agents
In
app/src/main/java/dev/arkbuilders/arkmemo/ui/viewmodels/ArkMediaPlayerViewModel.kt
around lines 121 to 123, the Log.d call inside computeFftMagnitude() runs very
frequently and can degrade performance due to excessive logging and string
allocations. To fix this, wrap the Log.d call with a conditional check using
Log.isLoggable(TAG, Log.DEBUG) or guard it with BuildConfig.DEBUG to ensure the
log executes only when debug logging is enabled.
|
@shubertm Please re-revew. |
| private var lastPaintColor = paintColor | ||
| private var strokeWidth = Size.TINY.value | ||
|
|
||
| private val tag = "GraphicNotesViewModel" |
There was a problem hiding this comment.
This can also be a constant
|
|
||
| companion object { | ||
| private const val CURRENT_FRAGMENT_TAG = "current fragment tag" | ||
| private const val CURRENT_FRAGMENT_TAG = "current_fragment_tag" |
There was a problem hiding this comment.
Better to remove "tag" at the end
| note: Note, | ||
| parentResId: ResourceId? = null, | ||
| ) { | ||
| Log.d(TAG, "add note with title: ${note.title} resId: ${note.resource?.id}") |
There was a problem hiding this comment.
Let's use resource.name for note.title. That should be the actual metadata not the note content
tuancoltech
force-pushed
the
feat/improve_logging
branch
from
a39de21 to
5f5a2bb
Compare
tuancoltech
had a problem deploying
to
Development
GitHub Actions
Failure
tuancoltech
force-pushed
the
feat/improve_logging
branch
from
5f5a2bb to
1ab66de
Compare
|
@shubertm Is it ok to merge this now? |
2 participants
Description
Improve logging by placing logs in core functions
Ticket
https://app.asana.com/1/1207783906637200/project/1207819682524134/task/1208764211704004
Summary by CodeRabbit