Conversation
Create a `getPageIndex(pageId)` extension function for the `Notebook` class to centralize the logic for finding a page's index. Corrected in Toolbar.kt how page number are retrieved, fixed issue #192
Contributor
There was a problem hiding this comment.
Pull request overview
This PR refactors page index retrieval logic by introducing a centralized Notebook.getPageIndex(pageId) extension function and fixes a critical NullPointerException reported in issue #192.
Changes:
- Created
Notebook.getPageIndex(pageId)extension function to centralize the logic for finding a page's index within a notebook - Fixed NullPointerException in Toolbar.kt by adding proper null-safe handling when retrieving page numbers
- Improved null safety in AppRepository.kt navigation methods (
getNextPageIdFromBookAndPageandgetPreviousPageIdFromBookAndPage)
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| app/src/main/java/com/ethran/notable/data/db/Notebook.kt | Added getPageIndex() extension function that wraps pageIds.indexOf() |
| app/src/main/java/com/ethran/notable/editor/ui/toolbar/Toolbar.kt | Fixed NPE by using safe calls and fallback values when computing page numbers; cached repository instance in remember block |
| app/src/main/java/com/ethran/notable/ui/views/PagesView.kt | Refactored to use new extension function; removed intermediate pageIds variable and access book.pageIds directly |
| app/src/main/java/com/ethran/notable/data/AppRepository.kt | Improved null handling and index validation; refactored to use new extension function |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
app/src/main/java/com/ethran/notable/editor/ui/toolbar/Toolbar.kt
Outdated
Show resolved
Hide resolved
Wrap AppRepository instantiation in `remember {}` to avoid creating a new instance on every recomposition.
- Use `rememberCoroutineScope` instead of creating a new `CoroutineScope` on each recomposition. - Hoist `stringResource` calls to the top level of the composable to avoid repeated lookups. - Launch export coroutines on `Dispatchers.IO` directly from the UI thread scope.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
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
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.
Create a
getPageIndex(pageId)extension function for theNotebookclass to centralize the logic for finding a page's index.Corrected in Toolbar.kt how page number are retrieved, fixed issue #192