Add Quillpad import and fix Notally import#978
Merged
Conversation
Contributor
📝 WalkthroughWalkthroughThis pull request adds import support for Quillpad and Notally applications alongside existing Google Keep and Evernote importers. Changes include: defining Quillpad backup data models, implementing ZIP extraction and JSON deserialization, introducing a Display interface to the ImportSource enum, extending import infrastructure utilities, updating the settings UI with a Notally info dialog, and adding comprehensive tests and documentation. Changes
Sequence Diagram(s)sequenceDiagram
participant User as User
participant UI as SettingsFragment
participant Importer as QuillpadImporter
participant ZIP as ZIP Handler
participant JSON as JSON Parser
participant Converter as Converter<br/>(toBaseNote)
participant DB as Database/Storage
User->>UI: Select "Quillpad" from import sources
UI->>Importer: import(uri, destination, progress)
Importer->>ZIP: Extract ZIP from contentResolver
ZIP->>ZIP: Validate path traversal safety
ZIP->>Importer: Extracted files + backup.json
Importer->>JSON: Deserialize backup.json
JSON->>Importer: QuillpadBackup object
Importer->>Converter: Convert each note via toBaseNote()
Converter->>Converter: Parse markdown, categorize attachments
Converter->>Converter: Map notebooks & tags to labels
Converter->>Importer: BaseNote objects
Importer->>DB: Store notes & media files
DB->>UI: Return import results
UI->>User: Display completion
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~28 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (5)
app/src/main/java/com/philkes/notallyx/utils/backup/ImportExtensions.kt (1)
69-90: Keep the projection explicit for required fields.
getOptionalColumns()currently returns whatever the source table happens to expose, buttoBaseNote()still hard-requires a fixed core (id,type,folder,title,body, etc.). Starting from an explicit required projection and appending only truly optional columns would make unsupported schemas fail earlier and more predictably.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/java/com/philkes/notallyx/utils/backup/ImportExtensions.kt` around lines 69 - 90, getOptionalColumns currently returns whatever columns exist which can hide missing required fields; change it to start from an explicit required projection (e.g., the fixed core fields used by toBaseNote such as id, type, folder, title, body) and then append only truly optional columns found in the DB; validate that all required columns are present and throw or log a clear error if any required column (referenced by toBaseNote) is missing, and keep your existing special-case for "body" using SUBSTR with MAX_BODY_CHAR_LENGTH when building the final projection array in getOptionalColumns.app/src/test/kotlin/com/philkes/notallyx/data/imports/NotesImporterTest.kt (1)
39-145: Add a Notally backup regression case to this suite.This PR fixes Notally import behavior, but the runnable coverage here still only exercises Google Keep / Evernote / Quillpad. A Notally fixture is the regression that matters most now—especially for the new
reminder→remindersfallback inImportExtensions.kt.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/test/kotlin/com/philkes/notallyx/data/imports/NotesImporterTest.kt` around lines 39 - 145, Add a regression test for Notally by adding a new `@Test` that calls testImport(ImportSource.NOTALLY, <expectedCount>) (or testImport(prepareImportSources(ImportSource.NOTALLY), ImportSource.NOTALLY, <expectedCount>)) and update prepareImportSources(importSource: ImportSource) to handle ImportSource.NOTALLY returning the Notally fixture file name (e.g., "notally_export.zip" or your fixture name); ensure a corresponding Notally fixture is present in the test resources so the NotesImporter code path (and the ImportExtensions.kt reminder → reminders fallback) is exercised.app/src/main/java/com/philkes/notallyx/presentation/activity/main/fragment/settings/SettingsFragment.kt (1)
541-558: Avoid re-deriving the importer from the dialog index.
ImportSource.entries[which - 1]works only while the dialog order stays perfectly aligned with the enum declaration. A later insert/reorder will silently launch the wrong importer. It’s safer to keep a single list of chooser items and read the selectedImportSource?directly from that list.♻️ One way to make the mapping explicit
- val notallyItem = - mutableListOf( - object : Display { - override fun getTextId(): Int { - return R.string.notally - } - - override fun getIconId(): Int { - return R.drawable.icon_notally - } - } - ) + val chooserItems: List<Pair<Display, ImportSource?>> = + listOf( + object : Display { + override fun getTextId(): Int = R.string.notally + override fun getIconId(): Int = R.drawable.icon_notally + } to null + ) + ImportSource.entries.map { it to it } MaterialAlertDialogBuilder(requireContext()) .setTitle(R.string.choose_other_app) .setAdapter( TextWithIconAdapter( requireContext(), - notallyItem + ImportSource.entries.toMutableList(), + chooserItems.map { it.first }, { item -> getString(item.getTextId()) }, Display::getIconId, ) ) { _, which -> - if (which == 0) { + val selected = chooserItems[which].second + if (selected == null) { MaterialAlertDialogBuilder(requireContext()) .setMessage( getString( @@ - selectedImportSource = ImportSource.entries[which - 1] + selectedImportSource = selected🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/java/com/philkes/notallyx/presentation/activity/main/fragment/settings/SettingsFragment.kt` around lines 541 - 558, The code currently maps the dialog selection back to an importer using ImportSource.entries[which - 1], which will break if enum order or dialog list changes; instead build and reuse the actual chooser list used to create the adapter (the list passed in alongside notallyItem and Display::getIconId) — e.g., create a local val chooserList = notallyItem + ImportSource.entries.toMutableList() and pass that to setAdapter, then set selectedImportSource by reading chooserList[which] (or chooserList[which - 1] if you keep the notallyItem header) so the selected value is taken directly from the same list rather than re-deriving via ImportSource.entries. Ensure you update the selection logic around selectedImportSource and the special-case check for the notallyItem header to use the same chooserList indexing.app/src/main/java/com/philkes/notallyx/data/imports/quillpad/QuillpadImporter.kt (2)
77-80: Unused variables:noteReminders,noteTags, andtagMap.These variables are computed but never used. The
toBaseNotefunction accessesquillpadNote.tagsandquillpadNote.remindersdirectly from the embedded note data instead. Either remove the unused variables or use them if the intent was to join data via IDs.♻️ Remove unused variables
val notebookMap = quillpadBackup.notebooks.associate { it.id to it.name } - val noteReminders = quillpadBackup.reminders.groupBy { it.noteId } - val noteTags = quillpadBackup.joins.groupBy { it.noteId } - val tagMap = quillpadBackup.tags.associate { it.id to it.name } val total = quillpadBackup.notes.size🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/java/com/philkes/notallyx/data/imports/quillpad/QuillpadImporter.kt` around lines 77 - 80, Remove or use the unused variables noteReminders, noteTags, and tagMap in QuillpadImporter.kt: either delete the lines that compute noteReminders, noteTags, and tagMap (since toBaseNote reads quillpadNote.tags and quillpadNote.reminders directly), or modify toBaseNote (or its caller) to look up tags and reminders by noteId using noteTags, noteReminders and resolve tag names via tagMap so those computed maps are actually consumed; reference noteReminders, noteTags, tagMap and the toBaseNote function when making the change.
43-48: Return type mismatch with interface.The method returns
Pair<List<BaseNote>, File>(non-nullFile), but theExternalImporterinterface declaresPair<List<BaseNote>, File?>. While this is type-safe (covariant), consider matching the interface signature exactly for consistency, or updating the interface if all importers always return a non-nullFile.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/java/com/philkes/notallyx/data/imports/quillpad/QuillpadImporter.kt` around lines 43 - 48, The QuillpadImporter.import signature currently returns Pair<List<BaseNote>, File> but the ExternalImporter interface declares Pair<List<BaseNote>, File?>; update QuillpadImporter.import to return Pair<List<BaseNote>, File?> (nullable File) to match the interface (or, if you intentionally guarantee a non-null File across all importers, update the ExternalImporter interface instead); adjust any usages of QuillpadImporter.import or the returned pair to handle a nullable File, and ensure the method implementation returns null where appropriate when no File is produced.
🤖 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/philkes/notallyx/data/imports/quillpad/QuillpadBackup.kt`:
- Around line 3-14: The file unnecessarily uses the internal serialization
annotation; remove the import kotlinx.serialization.InternalSerializationApi and
delete all usages of `@InternalSerializationApi` on the data classes (e.g.,
QuillpadBackup and its related QuillpadNote, QuillpadNotebook, QuillpadTag,
QuillpadReminder, QuillpadJoin classes) so they rely only on `@Serializable` and
primitive/default properties remain unchanged; ensure only the
kotlinx.serialization.Serializable import remains and the classes compile
without the internal annotation.
In
`@app/src/main/java/com/philkes/notallyx/data/imports/quillpad/QuillpadImporter.kt`:
- Around line 126-130: All Quillpad timestamps must be normalized to
milliseconds consistently; verify the source (Quillpad timestamps) and, if they
are in seconds, multiply every timestamp by 1000 before using or wrapping with
Date(), specifically change the timestamp expression used when building
AudioAttachment (currently using modifiedDate * 1000) and update the
Date(it.date) call and the places that pass creationDate and modifiedDate
directly so they all use the same normalized value (e.g., normalizeCreated =
creationDate * 1000, normalizeModified = modifiedDate * 1000) before
constructing Date(...) or assigning to timestamp fields in QuillpadImporter.kt.
In `@app/src/main/java/com/philkes/notallyx/utils/IOExtensions.kt`:
- Around line 290-300: The File.moveAllFiles extension currently ignores
renameTo() failures causing silent partial moves; update moveAllFiles to check
each file.renameTo(targetFile) result and fail fast on false by throwing an
IOException (or propagate an error) with context (source file and targetFile) so
callers like the Quillpad import path see the failure; ensure to keep the
mkdirs() creation logic but validate renameTo for every file in listFiles()
inside moveAllFiles and surface the error instead of swallowing it.
In `@app/src/main/java/com/philkes/notallyx/utils/NotallyUtils.kt`:
- Around line 18-30: The frequency mapping inside
NotallyReminderJson?.toNotallyXReminder() currently only handles "DAILY" and
"MONTHLY" and must be extended: add a "WEEKLY" case mapping to Repetition(1,
RepetitionTimeUnit.WEEKS) and expand the when(...) on getSafeString("frequency")
to handle other Notally frequency tokens (or normalize case with .uppercase())
rather than falling through to null; for unknown tokens, either translate them
to the closest Repetition/RepetitionTimeUnit or emit a warning/log so the
recurrence isn't silently lost when constructing the Reminder in
toNotallyXReminder().
In `@app/src/test/kotlin/com/philkes/notallyx/data/imports/NotesImporterTest.kt`:
- Around line 31-32: The call to
application.getExternalFilesDir(Environment.MEDIA_MOUNTED) is incorrect and dead
code; either remove that line or change the argument to null so it requests the
app-specific external files root. Locate the
ApplicationProvider.getApplicationContext() usage in NotesImporterTest.kt and
update the getExternalFilesDir(...) call (or delete it) and ensure tests
continue to rely on getCurrentMediaRoot() for external media access.
In
`@app/src/test/kotlin/com/philkes/notallyx/data/imports/quillpad/QuillpadImporterTest.kt`:
- Around line 171-199: The test failing because it expects non-standard
"image/jpg" — update the MIME normalization in the importer where it builds the
MIME type (the code around the MIME creation in QuillpadImporter, currently
doing "image/$extension" near the MIME derivation lines) to map "jpg" to "jpeg"
(and similarly "tif" to "tiff" if desired) before composing the MIME string,
then keep the test expecting standard "image/jpeg" (or alternatively change the
test expectation to "image/jpeg"); locate the logic in QuillpadImporter (the
function that converts attachments into FileAttachment and sets the MIME) and
normalize extensions like "jpg" -> "jpeg" prior to forming the MIME type.
- Around line 277-281: Timestamps for reminders are being passed as seconds into
Date(...) causing a unit mismatch; update the QuillpadImporter code where
reminders are converted (the usage of Date(it.date) in the reminder mapping
inside parseToBaseNote()/QuillpadImporter) to multiply the incoming epoch
seconds by 1000 (Date(it.date * 1000)) to match the audio conversion pattern
(modifiedDate * 1000), and update the test expectation in QuillpadImporterTest
to expect Date(1776012540000L) (or 1776012540 * 1000) for the reminder.
In `@documentation/docs/features/import-other-apps.mdx`:
- Around line 16-22: Update the "Available Imports" list in the documentation by
adding the JSON importer entry and align the combined files label with the
in-app chooser; specifically, add an item for the JSON importer (e.g., "JSON
Importer") to the list and change the existing "Plain Text Files" / "Markdown
Files" entries to the single combined label used in-app ("Markdown / Plain Text
Files") so the docs match the chooser options shown to users.
---
Nitpick comments:
In
`@app/src/main/java/com/philkes/notallyx/data/imports/quillpad/QuillpadImporter.kt`:
- Around line 77-80: Remove or use the unused variables noteReminders, noteTags,
and tagMap in QuillpadImporter.kt: either delete the lines that compute
noteReminders, noteTags, and tagMap (since toBaseNote reads quillpadNote.tags
and quillpadNote.reminders directly), or modify toBaseNote (or its caller) to
look up tags and reminders by noteId using noteTags, noteReminders and resolve
tag names via tagMap so those computed maps are actually consumed; reference
noteReminders, noteTags, tagMap and the toBaseNote function when making the
change.
- Around line 43-48: The QuillpadImporter.import signature currently returns
Pair<List<BaseNote>, File> but the ExternalImporter interface declares
Pair<List<BaseNote>, File?>; update QuillpadImporter.import to return
Pair<List<BaseNote>, File?> (nullable File) to match the interface (or, if you
intentionally guarantee a non-null File across all importers, update the
ExternalImporter interface instead); adjust any usages of
QuillpadImporter.import or the returned pair to handle a nullable File, and
ensure the method implementation returns null where appropriate when no File is
produced.
In
`@app/src/main/java/com/philkes/notallyx/presentation/activity/main/fragment/settings/SettingsFragment.kt`:
- Around line 541-558: The code currently maps the dialog selection back to an
importer using ImportSource.entries[which - 1], which will break if enum order
or dialog list changes; instead build and reuse the actual chooser list used to
create the adapter (the list passed in alongside notallyItem and
Display::getIconId) — e.g., create a local val chooserList = notallyItem +
ImportSource.entries.toMutableList() and pass that to setAdapter, then set
selectedImportSource by reading chooserList[which] (or chooserList[which - 1] if
you keep the notallyItem header) so the selected value is taken directly from
the same list rather than re-deriving via ImportSource.entries. Ensure you
update the selection logic around selectedImportSource and the special-case
check for the notallyItem header to use the same chooserList indexing.
In `@app/src/main/java/com/philkes/notallyx/utils/backup/ImportExtensions.kt`:
- Around line 69-90: getOptionalColumns currently returns whatever columns exist
which can hide missing required fields; change it to start from an explicit
required projection (e.g., the fixed core fields used by toBaseNote such as id,
type, folder, title, body) and then append only truly optional columns found in
the DB; validate that all required columns are present and throw or log a clear
error if any required column (referenced by toBaseNote) is missing, and keep
your existing special-case for "body" using SUBSTR with MAX_BODY_CHAR_LENGTH
when building the final projection array in getOptionalColumns.
In `@app/src/test/kotlin/com/philkes/notallyx/data/imports/NotesImporterTest.kt`:
- Around line 39-145: Add a regression test for Notally by adding a new `@Test`
that calls testImport(ImportSource.NOTALLY, <expectedCount>) (or
testImport(prepareImportSources(ImportSource.NOTALLY), ImportSource.NOTALLY,
<expectedCount>)) and update prepareImportSources(importSource: ImportSource) to
handle ImportSource.NOTALLY returning the Notally fixture file name (e.g.,
"notally_export.zip" or your fixture name); ensure a corresponding Notally
fixture is present in the test resources so the NotesImporter code path (and the
ImportExtensions.kt reminder → reminders fallback) is exercised.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7252e7cb-2a33-4425-877b-87337a5a0025
⛔ Files ignored due to path filters (2)
app/src/test/resources/imports/quillpad/quillpad_backup.zipis excluded by!**/*.zipapp/translations.xlsxis excluded by!**/*.xlsx
📒 Files selected for processing (21)
README.mdapp/build.gradle.ktsapp/src/main/java/com/philkes/notallyx/data/imports/NotesImporter.ktapp/src/main/java/com/philkes/notallyx/data/imports/google/GoogleKeepImporter.ktapp/src/main/java/com/philkes/notallyx/data/imports/google/GoogleKeepNote.ktapp/src/main/java/com/philkes/notallyx/data/imports/quillpad/QuillpadBackup.ktapp/src/main/java/com/philkes/notallyx/data/imports/quillpad/QuillpadImporter.ktapp/src/main/java/com/philkes/notallyx/data/model/Converters.ktapp/src/main/java/com/philkes/notallyx/data/model/Reminder.ktapp/src/main/java/com/philkes/notallyx/presentation/activity/main/fragment/settings/SettingsFragment.ktapp/src/main/java/com/philkes/notallyx/presentation/view/misc/TextWithIconAdapter.ktapp/src/main/java/com/philkes/notallyx/utils/IOExtensions.ktapp/src/main/java/com/philkes/notallyx/utils/NotallyUtils.ktapp/src/main/java/com/philkes/notallyx/utils/backup/ImportExtensions.ktapp/src/main/res/drawable/icon_notally.xmlapp/src/main/res/drawable/icon_quillpad.xmlapp/src/main/res/values/strings.xmlapp/src/test/kotlin/com/philkes/notallyx/data/imports/NotesImporterTest.ktapp/src/test/kotlin/com/philkes/notallyx/data/imports/quillpad/QuillpadImporterTest.ktdocumentation/docs/features/import-other-apps.mdxdocumentation/docs/intro.md
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
app/src/main/java/com/philkes/notallyx/utils/MiscExtensions.kt (1)
142-142: Consider explicit return type for public API stability.This is fine as-is; adding an explicit return type keeps the contract clearer for future edits.
Suggested tweak
-fun Seconds.toMillis() = this * 1000 +fun Seconds.toMillis(): Long = this * 1000L🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/java/com/philkes/notallyx/utils/MiscExtensions.kt` at line 142, Add an explicit return type to the extension function Seconds.toMillis to stabilize the public API contract; update the function signature for Seconds.toMillis (the extension) to declare the return type (e.g., : Long) so callers and future edits have a clear, fixed return type.
🤖 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/philkes/notallyx/data/imports/quillpad/QuillpadImporter.kt`:
- Around line 185-213: The unzip function currently creates a ZipInputStream
(zis) without using Kotlin's use{} so exceptions can leak the stream; wrap
ZipInputStream(inputStream) in a use { zis -> ... } block around the entire
extraction loop in unzip, move zis.closeEntry() to run inside the loop after
processing each zipEntry, and remove the manual zis.close()/closeEntry() calls
at the end; keep the existing FileOutputStream(newFile).use { ... } and continue
using newFile(...) to create files so resources are properly closed on
exceptions.
---
Nitpick comments:
In `@app/src/main/java/com/philkes/notallyx/utils/MiscExtensions.kt`:
- Line 142: Add an explicit return type to the extension function
Seconds.toMillis to stabilize the public API contract; update the function
signature for Seconds.toMillis (the extension) to declare the return type (e.g.,
: Long) so callers and future edits have a clear, fixed return type.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d5e3f1e6-06b4-42fa-84b8-b8900ed912eb
📒 Files selected for processing (7)
app/src/main/java/com/philkes/notallyx/data/imports/quillpad/QuillpadBackup.ktapp/src/main/java/com/philkes/notallyx/data/imports/quillpad/QuillpadImporter.ktapp/src/main/java/com/philkes/notallyx/utils/IOExtensions.ktapp/src/main/java/com/philkes/notallyx/utils/MiscExtensions.ktapp/src/test/kotlin/com/philkes/notallyx/data/imports/NotesImporterTest.ktapp/src/test/kotlin/com/philkes/notallyx/data/imports/quillpad/QuillpadImporterTest.ktdocumentation/docs/features/import-other-apps.mdx
✅ Files skipped from review due to trivial changes (3)
- documentation/docs/features/import-other-apps.mdx
- app/src/test/kotlin/com/philkes/notallyx/data/imports/NotesImporterTest.kt
- app/src/main/java/com/philkes/notallyx/data/imports/quillpad/QuillpadBackup.kt
🚧 Files skipped from review as they are similar to previous changes (2)
- app/src/main/java/com/philkes/notallyx/utils/IOExtensions.kt
- app/src/test/kotlin/com/philkes/notallyx/data/imports/quillpad/QuillpadImporterTest.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.
Fixes #938
Closes #964
Summary by CodeRabbit
New Features
Documentation
Tests