fix(ios): SQLite transactions, credential trim, onboarding nav icon, safe-area - #1130
Merged
Merged
Conversation
…x 20+ second library load On iOS every driver.execute() call without an explicit transaction is an implicit auto-commit that triggers its own WAL fsync (~5-15 ms on NAND flash). For a 500-book ABS library this produced 1000+ fsyncs (500 INSERT OR IGNORE + 500 UPDATE from replaceAllForLibrary, plus up to 500 more from the per-item progress update loop) totalling 20+ seconds before the spinner cleared. Android is unaffected because Room's @transaction wraps the default replaceAllForLibrary implementation in a single BEGIN/COMMIT automatically. Fix: - Add IosTransaction.kt with a SqlDriver.withTransaction() suspend extension that wraps a block in BEGIN IMMEDIATE / COMMIT / ROLLBACK. - Override replaceAllForLibrary in IosLibraryItemDao, IosSeriesDao, and IosCollectionDao to issue all SQL inside one transaction and fire invalidator.invalidate() only once at the end (instead of once per row for updateMetadata). - Add LibraryItemDao.batchUpdateReadingProgressFromServer (default: loops + @transaction for Android; iOS override: single explicit transaction) and replace the per-item loop in IosLibraryRefresherImpl with a single batch call. - Add IosLibraryItemDaoTest covering replaceAllForLibrary correctness (inserts, stale- item deletion, metadata update, local-progress preservation) and batch progress update correctness (multi-item update, last-write-wins staleness guard). Expected improvement: 500-book refresh ~200 ms on iOS (down from 20+ s).
…thenticating A physical keyboard on iOS (or autocorrect on any platform) can insert a trailing space into a text field, causing "test " to be sent instead of "test" and returning 401 from the server. The URL field was already trimmed; the credentials were not. Fix: call .trim() on username and password in doAuthenticate() and connectWebdav() before passing them to the authenticator. Regression test: AddSourceViewModelTest asserts that leading/trailing whitespace is stripped from both fields before the authenticate() call.
When the app launches with no source configured the SourceOnboardingHost is shown at root — there is nowhere to navigate back to, so showing the back arrow is confusing and non-functional. AddSourceScreen and SourceTypePickerScreen gain a showNavigationIcon parameter (default true, so all existing callers are unaffected). SourceOnboardingHost gains canNavigateBack (default true) and forwards it to both screens. HomeScreen passes canNavigateBack=false when showing the onboarding flow. On Android, SourceNavGraph passes showNavigationIcon=cameFromSettings to both screens, hiding the icon on first-run and showing it when reached from Settings.
Previously only the bottom edge was excluded from safe-area insets. Switching to .ignoresSafeArea(.all) lets the Compose layer fill the full screen including the status-bar region, matching the Android window-insets behaviour.
…g onboarding Android: SourceTypePickerScreenTest adds backButton_shownByDefault and backButton_hiddenWhenShowNavigationIconFalse to lock the regression that the nav_back button disappears when canNavigateBack=false is passed through SourceOnboardingHost. iOS: OnboardingNavIconTests (iosAppTests target) drives the same claim through XCUIApplication — verifies nav_back is absent on both the picker and AddSource screens during first-run onboarding (app launched with --RIFFLE_RESET_FOR_TESTS and no seeded source).
… column count Two bugs surfaced in code review: 1. IosCollectionDao and IosSeriesDao gated invalidator.invalidate() on `isNotEmpty()`, so a refresh that removes ALL series/collections from the server left deleted rows visible in the library indefinitely. Fix: invalidate unconditionally after every replaceAllForLibrary call (the transaction already ran the DELETEs, so the UI must be notified). 2. insertOrIgnoreRaw (and the older insertOrIgnore) passed 24 as the parameter count to SqlDriver.execute() but ALL_COLS/PLACEHOLDERS have 25 columns. The 25th binding (progressServerUpdatedAt) was silently dropped, storing 0 for that column on every newly inserted row. Fix: pass 25 in both call sites.
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.
Changes
fix(ios): wrap batch DAO writes in explicit SQLite transactions — 20+ second library load
On iOS, SQLite operates without a shared Room
@Transactionwrapper.Every
driver.execute()in a loop triggered its own WAL fsync (~5-15 ms on NAND), turning a 500-book library refresh into a 20+ second hang.Fix: new
SqlDriver.withTransactionextension (BEGIN IMMEDIATE / COMMIT / ROLLBACK) wraps the full batch in a single fsync. Applied toIosLibraryItemDao.replaceAllForLibrary,IosSeriesDao.replaceAllForLibrary, andIosCollectionDao.replaceAllForLibrary.IosLibraryRefresherImplbatches per-item progress updates via the newbatchUpdateReadingProgressFromServerDAO method.Regression coverage:
IosLibraryItemDaoTest(10 tests incore:database iosSimulatorArm64Test).fix(add-source): trim whitespace from username and password before authenticating
The URL field was already trimmed; credentials were not. iOS can insert invisible trailing whitespace (autocorrect, physical keyboard layout mismatch), causing
"test "to reach the server as the password and return 401. Fix:.trim()onusernameandpasswordindoAuthenticate()andconnectWebdav().Regression test:
AddSourceViewModelTest.onConnect trims leading and trailing whitespace from username and password before authenticating(androidHostTest).iOS path:
AddSourceViewModeliscommonMain; iOS exercises the same trim code path. No separate iOS test needed per the ViewModel-with-Room-coupled-constructor exception — but the trim itself is pureString.trim()in shared code.fix(source-ui): hide back navigation icon during first-run onboarding
During first-run onboarding (no source configured) the back arrow was shown but had nowhere to go.
SourceTypePickerScreenandAddSourceScreengainshowNavigationIcon(defaulttrue).SourceOnboardingHostgainscanNavigateBack(defaulttrue) forwarded to both screens.HomeScreenpassescanNavigateBack = false. On Android,SourceNavGraphpassesshowNavigationIcon = cameFromSettings.Tests:
SourceTypePickerScreenTest.backButton_hiddenWhenShowNavigationIconFalse(androidTest)OnboardingNavIconTests.testBackButtonAbsentOnPickerDuringOnboarding+testBackButtonAbsentOnAddSourceDuringOnboarding(iosAppTests target)fix(ios): extend safe-area inset to all edges in ContentView
ContentViewpreviously only excluded the bottom safe-area edge. Changed to.ignoresSafeArea(.all)so the Compose layer fills the full screen including the status-bar region, matching Android window-insets behaviour.fix(ios-db): always invalidate after replaceAllForLibrary; fix INSERT column count
Two bugs found in code review:
IosCollectionDaoandIosSeriesDaogatedinvalidator.invalidate()onisNotEmpty(). A server refresh that removes ALL series/collections left deleted rows visible in the library indefinitely. Fix: invalidate unconditionally.insertOrIgnoreRaw(and olderinsertOrIgnore) passed24as the SQLite parameter count, butALL_COLS/PLACEHOLDERShas 25 columns. The 25th binding (progressServerUpdatedAt) was silently dropped, storing0on every newly inserted row. Fix: pass25in both call sites.iOS call path
IosLibraryRefresherImpl→IosLibraryItemDao.replaceAllForLibrary→SqlDriver.withTransactionAddSourceViewModel.doAuthenticate(commonMain) →username.trim()/password.trim()→AbsSourceAdapter.authenticateHomeScreen→SourceOnboardingHost(canNavigateBack=false)→SourceTypePickerScreen(showNavigationIcon=false)/AddSourceScreen(showNavigationIcon=false)🤖 Generated with Claude Code