Conversation
…velop the app. Now it's mostly a big mess, but it goes in right direction
--AI--
Refactor navigation and settings management, and introduce a centralized `NotableApp` component.
Key changes:
- **Navigation Refactor**: Replaced the monolithic `Router.kt` with a state-managed `NotableAppState` and a modular `NotableNavHost`.
- **Destination Objects**: Defined explicit `NavigationDestination` objects (e.g., `EditorDestination`, `LibraryDestination`) for type-safe routing.
- **Dependency Injection**: Integrated Dagger Hilt for dependency management and updated `KvProxy` to use constructor injection.
- **ViewModel Implementation**: Introduced `SettingsViewModel` and `QuickNavViewModel` to decouple UI logic from data persistence.
- **Gesture Decoupling**: Moved gesture logic (e.g., `QuickNavGesture`, `EditorGestureReceiver`) into a dedicated `gestures` package.
- **UI Enhancements**:
- Refactored `SettingsView` and `WelcomeView` into stateless components with dedicated Previews.
- Updated `QuickNav` to use a ViewModel-driven state pattern.
- Improved `BreadCrumb` and `ShowPagesRow` for better reusability.
- **Clean up**: Removed unused dependencies and simplified various UI component signatures.
There was a problem hiding this comment.
Pull request overview
This PR performs a broad UI/navigation refactor: it replaces the old Router-based navigation with a new NotableApp + NotableNavHost setup, introduces a NavigationDestination pattern for routes, and starts integrating Hilt for DI while reshaping several Compose screens into more “stateless content + stateful wrapper” patterns.
Changes:
- Introduces new navigation architecture (
NotableAppState,NotableNavHost) and removes the legacyRouter. - Adds Hilt (application + activity entry point, DI-ready
KvProxy/KvRepository) and starts usinghiltViewModel()in Settings. - Refactors multiple UI components (Welcome/Settings/QuickNav/Breadcrumb/ToolbarMenu) and adds Compose previews/preview-safe rendering.
Reviewed changes
Copilot reviewed 40 out of 42 changed files in this pull request and generated 23 comments.
Show a summary per file
| File | Description |
|---|---|
| build.gradle | Adds Hilt Gradle plugin (apply false) at the root. |
| app/build.gradle | Applies Hilt plugin and adds Hilt dependencies. |
| app/src/main/res/values/strings.xml | Adds new gesture action label (“Select”). |
| app/src/main/res/values-pl/strings.xml | Adds Polish translation for the new gesture label. |
| app/src/main/java/com/ethran/notable/ui/views/WelcomeView.kt | Refactors into stateful wrapper + WelcomeContent, lifecycle-based refresh, adds previews. |
| app/src/main/java/com/ethran/notable/ui/views/SystemInformation.kt | Converts to callback-based navigation, adds a destination object and preview annotation. |
| app/src/main/java/com/ethran/notable/ui/views/Settings.kt | Refactors to ViewModel-driven settings with stateless SettingsContent, adds previews. |
| app/src/main/java/com/ethran/notable/ui/views/PagesView.kt | Introduces PagesDestination, updates routes and breadcrumb usage. |
| app/src/main/java/com/ethran/notable/ui/views/LogView.kt | Introduces BugReportDestination. |
| app/src/main/java/com/ethran/notable/ui/views/HomeView.kt | Introduces LibraryDestination, updates breadcrumb and editor navigation wiring. |
| app/src/main/java/com/ethran/notable/ui/viewmodels/SettingsViewModel.kt | New Hilt ViewModel for settings persistence + update checking + gesture configuration list. |
| app/src/main/java/com/ethran/notable/ui/viewmodels/QuickNavViewModel.kt | New ViewModel to manage QuickNav state (favorites, breadcrumb, scrubber). |
| app/src/main/java/com/ethran/notable/ui/dialogs/NotebookConfig.kt | Updates breadcrumb API usage. |
| app/src/main/java/com/ethran/notable/ui/dialogs/FolderSelectionDialog.kt | Updates breadcrumb API usage. |
| app/src/main/java/com/ethran/notable/ui/components/ShowPagesRow.kt | Makes row stateless by pushing navigation/page creation out via callbacks. |
| app/src/main/java/com/ethran/notable/ui/components/QuickNav.kt | Refactors QuickNav to use a ViewModel + QuickNavContent split and adds preview. |
| app/src/main/java/com/ethran/notable/ui/components/PagePreview.kt | Makes preview rendering safe using LocalInspectionMode and uses Coil with a File model. |
| app/src/main/java/com/ethran/notable/ui/components/NotableApp.kt | New top-level app composable hosting nav + quick-nav gesture + anchor + snack bar. |
| app/src/main/java/com/ethran/notable/ui/components/GesturesSettings.kt | Refactors gesture settings to be stateless and configurable via models + callback. |
| app/src/main/java/com/ethran/notable/ui/components/GeneralSettings.kt | Refactors to use onSettingsChange callback instead of writing directly to KV. |
| app/src/main/java/com/ethran/notable/ui/components/DebugSettings.kt | Refactors to callback-based navigation/settings update. |
| app/src/main/java/com/ethran/notable/ui/components/BreadCrumb.kt | Refactors breadcrumb to take a folder list, and adds folder list helpers + preview. |
| app/src/main/java/com/ethran/notable/ui/components/Anchor.kt | Makes anchor navigation external via onClose and renames/downgrades helper visibility. |
| app/src/main/java/com/ethran/notable/ui/Router.kt | Removes legacy navigation/router implementation. |
| app/src/main/java/com/ethran/notable/navigation/NotableNavHost.kt | New NavHost implementation using *Destination objects and new routes. |
| app/src/main/java/com/ethran/notable/navigation/NotableAppState.kt | New app navigation state holder (QuickNav/anchor/start destination). |
| app/src/main/java/com/ethran/notable/navigation/NavigationDestination.kt | New interface for route definitions. |
| app/src/main/java/com/ethran/notable/gestures/QuickNavGesture.kt | Extracts the 3-finger QuickNav gesture detector into a reusable modifier. |
| app/src/main/java/com/ethran/notable/gestures/GestureState.kt | Moves gesture state/constants into a new gestures package. |
| app/src/main/java/com/ethran/notable/gestures/EditorGestureReceiver.kt | Moves/updates gesture receiver to new package and adds selection cues integration. |
| app/src/main/java/com/ethran/notable/editor/utils/eraser.kt | Adds calls intended to remove strokes as part of erase flows. |
| app/src/main/java/com/ethran/notable/editor/ui/toolbar/ToolbarMenu.kt | Refactors menu to be navigation-callback based and adds previews. |
| app/src/main/java/com/ethran/notable/editor/ui/toolbar/Toolbar.kt | Updates toolbar menu invocation and routes via new destinations + parent folder helper. |
| app/src/main/java/com/ethran/notable/editor/ui/toolbar/EraserToolbarButton.kt | Updates app-settings persistence call site to new KvProxy(KvRepository) constructor. |
| app/src/main/java/com/ethran/notable/editor/PageView.kt | Updates constant import to new gestures package. |
| app/src/main/java/com/ethran/notable/editor/EditorView.kt | Introduces EditorDestination and updates gesture receiver import path. |
| app/src/main/java/com/ethran/notable/data/db/Page.kt | Adds Page.getParentFolder(...) helper. |
| app/src/main/java/com/ethran/notable/data/db/Kv.kt | Makes KV repository/proxy DI-ready and updates KvProxy constructor signature. |
| app/src/main/java/com/ethran/notable/data/AppRepository.kt | Updates to new KvProxy(kvRepository) constructor. |
| app/src/main/java/com/ethran/notable/NotableApp.kt | Marks Application with @HiltAndroidApp. |
| app/src/main/java/com/ethran/notable/MainActivity.kt | Switches to NotableApp composable, adds @AndroidEntryPoint + injected KvProxy. |
| .idea/inspectionProfiles/Project_Default.xml | Expands Compose preview inspections to preview files. |
Files not reviewed (1)
- .idea/inspectionProfiles/Project_Default.xml: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
app/src/main/java/com/ethran/notable/navigation/NotableNavHost.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/com/ethran/notable/ui/views/SystemInformation.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/com/ethran/notable/ui/viewmodels/SettingsViewModel.kt
Outdated
Show resolved
Hide resolved
| } | ||
|
|
||
| fun shouldAnchorBeVisible(): Boolean { | ||
| return isQuickNavOpen && quickNavSourcePageId != currentPageId |
There was a problem hiding this comment.
shouldAnchorBeVisible() doesn’t check that quickNavSourcePageId is non-null. If that savedState value is missing/null while currentPageId is non-null, this will still return true and can show an Anchor that can’t navigate anywhere. Consider requiring quickNavSourcePageId != null as well.
| return isQuickNavOpen && quickNavSourcePageId != currentPageId | |
| return isQuickNavOpen && quickNavSourcePageId != null && quickNavSourcePageId != currentPageId |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 50 out of 53 changed files in this pull request and generated 10 comments.
Files not reviewed (2)
- .idea/deploymentTargetSelector.xml: Language not supported
- .idea/inspectionProfiles/Project_Default.xml: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
| val title = viewModel.getIssueTitle() | ||
| val body = uiState.finalMarkdown | ||
| val url = "https://github.com/ethran/notable/issues/new?" + | ||
| "title=${URLEncoder.encode("Bug: ${getTitle(description)}", "UTF-8")}" + | ||
| "&body=${URLEncoder.encode(rapportMarkdown(includeLogs, description), "UTF-8")}" | ||
| "title=${URLEncoder.encode("Bug: $title", "UTF-8")}" + | ||
| "&body=${URLEncoder.encode(body, "UTF-8")}" |
There was a problem hiding this comment.
submitBugReport() prepends "Bug: " to title, but BugReportViewModel.getIssueTitle() already returns a string starting with "Bug: ". This results in GitHub issues titled like "Bug: Bug: ...". Either return a raw title from the ViewModel (no prefix) or stop prepending here.
| viewModelScope.launch(Dispatchers.IO) { | ||
| val page = runCatching { pageRepository.getById(currentPageId) }.getOrNull() | ||
| val folderList = getFolderList(appRepository, page) | ||
|
|
||
| // Read favorites from your database/preferences | ||
| val currentSettings = GlobalAppSettings.current | ||
| val favorites = currentSettings.quickNavPages | ||
| val isFavorite = favorites.contains(currentPageId) | ||
|
|
||
| val favoritePagesDb = appRepository.pageRepository.getByIds(favorites) | ||
|
|
||
| _uiState.update { state -> | ||
| state.copy( | ||
| folderId = page?.parentFolderId, | ||
| bookId = page?.notebookId, | ||
| isCurrentPageFavorite = isFavorite, | ||
| favoritePages = favoritePagesDb, | ||
| isLoading = false | ||
| ) | ||
| } | ||
|
|
||
| // Load Scrubber data if it belongs to a book | ||
| page?.notebookId?.let { loadBookData(it, currentPageId, favorites) } | ||
|
|
||
|
|
||
| viewModelScope.launch(Dispatchers.IO) { | ||
|
|
||
| _uiState.update { state -> | ||
| state.copy( | ||
| folderId = page?.parentFolderId, | ||
| breadcrumbFolders = folderList, // <-- PASS TO STATE | ||
| bookId = page?.notebookId, | ||
| isCurrentPageFavorite = isFavorite, | ||
| favoritePages = favoritePagesDb, | ||
| isLoading = false | ||
| ) | ||
| } | ||
| } |
There was a problem hiding this comment.
QuickNavViewModel.loadPageData() updates _uiState twice and also starts a nested viewModelScope.launch(Dispatchers.IO) inside an existing IO coroutine (lines 81-93). The inner launch is redundant and can cause unnecessary extra recompositions. Consolidate into a single _uiState.update { ... } and remove the nested coroutine.
| // TODO : change it!! | ||
| KvProxy(KvRepository(context)).setAppSettings( | ||
| GlobalAppSettings.current.copy(scribbleToEraseEnabled = isChecked) | ||
| ) |
There was a problem hiding this comment.
This click handler constructs a new KvRepository / KvProxy every toggle, using the current Compose context. That’s relatively heavy (Room access + permission check) and risks holding onto an Activity context. Prefer reusing an injected KvProxy (via Hilt) or at least remember/context.applicationContext so you don’t rebuild DB plumbing on every click.
| package com.ethran.notable.ui.views | ||
|
|
||
| import android.content.Context | ||
| import android.content.Intent |
There was a problem hiding this comment.
The file path is ui/viewmodels/BugReportGenerator.kt, but the declared package is com.ethran.notable.ui.views. This is legal, but it makes the codebase harder to navigate (and is easy to mis-import). Either move the file under ui/views/ or change the package to match the ui.viewmodels namespace.
| fun loadBook(bookId: String) { | ||
| viewModelScope.launch { | ||
| appRepository.bookRepository.getByIdLive(bookId).asFlow().collect { book -> | ||
| if (book != null) { | ||
| _uiState.update { it.copy( | ||
| bookId = bookId, | ||
| pageIds = book.pageIds, | ||
| openPageId = book.openPageId, | ||
| folderList = getFolderList(context, book.parentFolderId), | ||
| isLoading = false | ||
| ) } | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
PagesViewModel.loadBook() launches a new collect on the LiveData-backed flow every time it’s called, but never cancels the previous collector. Since PagesView calls loadBook(bookId) from LaunchedEffect(bookId), switching book IDs can leave multiple collectors running and emitting into _uiState. Consider tracking and cancelling the previous Job, or using flatMapLatest on a MutableStateFlow(bookId).
| fun newPageInBook(bookId: String, index: Int) { | ||
| appRepository.newPageInBook(bookId, index) | ||
| } |
There was a problem hiding this comment.
newPageInBook() calls appRepository.newPageInBook(...) synchronously without switching to Dispatchers.IO. This performs multiple DB operations and will run on the caller thread (often the UI thread via click handlers), risking jank/ANRs. Wrap this call in viewModelScope.launch(Dispatchers.IO) (and ideally return the new pageId via state/event).
app/src/main/java/com/ethran/notable/navigation/NotableNavHost.kt
Outdated
Show resolved
Hide resolved
| val uiState by viewModel.uiState.collectAsStateWithLifecycle() | ||
| val lifecycleOwner = LocalLifecycleOwner.current | ||
|
|
||
| // Initial load | ||
| LaunchedEffect(Unit) { | ||
| refresh() | ||
| } | ||
|
|
||
| // Refresh on RESUME (focus gain) | ||
| // Auto-refresh on Resume | ||
| DisposableEffect(lifecycleOwner) { | ||
| val observer = LifecycleEventObserver { _, event -> | ||
| if (event == Lifecycle.Event.ON_RESUME) { | ||
| refresh() | ||
| viewModel.refresh(context) | ||
| } | ||
| } | ||
| lifecycleOwner.lifecycle.addObserver(observer) | ||
| onDispose { lifecycleOwner.lifecycle.removeObserver(observer) } | ||
| } |
There was a problem hiding this comment.
SystemInformationView no longer triggers an initial refresh. Since the only refresh happens on Lifecycle.Event.ON_RESUME, the first time this screen is opened the uiState can remain null (and the UI stuck on “Loading device data…”) until the next resume event. Please call viewModel.refresh(context) once on first composition (e.g., LaunchedEffect(Unit)), or perform the initial refresh in the ViewModel init.
| val wholeLogsLength = URLEncoder.encode(formattedLogs, "UTF-8").length | ||
| val trimmedLogs = if (wholeLogsLength > availableSpace) { | ||
| // Binary search for optimal truncation point | ||
| var low = 0 | ||
| var high = wholeLogsLength | ||
| var bestLength = 0 | ||
|
|
||
| while (low <= high) { | ||
| val mid = (low + high) / 2 | ||
| val testLogs = formattedLogs.take(mid) | ||
| val testEncoded = URLEncoder.encode(testLogs, "UTF-8") | ||
|
|
||
| if (testEncoded.length <= availableSpace) { | ||
| bestLength = mid | ||
| low = mid + 1 | ||
| } else { | ||
| high = mid - 1 | ||
| } | ||
| } | ||
| formattedLogs.take(bestLength) | ||
| } else { |
There was a problem hiding this comment.
The log trimming binary search mixes encoded length and raw character length: high is set to wholeLogsLength (URL-encoded length) but you call formattedLogs.take(mid) where mid is treated as raw characters. This can fail to trim enough (or at all) because URL encoding expands the string. Consider binary searching on raw indices (0..formattedLogs.length) while comparing URLEncoder.encode(formattedLogs.take(mid), ...).length to availableSpace.
- Move navigation helper functions from `NotableNavHost` to `NotableAppState`. - Relocate `QuickNav` overlay from `NotableNavHost` to `NotableApp`. - Update `NotableAppState` to handle drawing state updates via a provided `CoroutineScope`. - Remove several experimental Compose annotations (`ExperimentalAnimationApi`, `ExperimentalFoundationApi`, `ExperimentalComposeUiApi`) across multiple files. - Ensure consistent use of destination argument constants in `NotableNavHost`.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 51 out of 54 changed files in this pull request and generated 3 comments.
Files not reviewed (2)
- .idea/deploymentTargetSelector.xml: Language not supported
- .idea/inspectionProfiles/Project_Default.xml: Language not supported
You can also share your feedback on Copilot code review. Take the survey.
| } | ||
|
|
||
| fun shouldAnchorBeVisible(): Boolean { | ||
| return isQuickNavOpen && quickNavSourcePageId != currentPageId |
There was a problem hiding this comment.
shouldAnchorBeVisible() currently requires isQuickNavOpen, which means the Anchor will never be shown after QuickNav is closed—even if the user jumped to a different page and quickNavSourcePageId is still set. Consider making visibility depend on quickNavSourcePageId != null && quickNavSourcePageId != currentPageId (and drive it off pageChangesSinceJump if that’s the new source of truth).
| return isQuickNavOpen && quickNavSourcePageId != currentPageId | |
| val sourcePageId = quickNavSourcePageId | |
| val currentId = currentPageId | |
| if (sourcePageId == null || sourcePageId == currentId) { | |
| return false | |
| } | |
| val savedStateHandle = navController.currentBackStackEntry?.savedStateHandle | |
| val pageChangesSinceJump = savedStateHandle?.get<Int>("pageChangesSinceJump") ?: 0 | |
| return pageChangesSinceJump > 0 |
| val appRepository = AppRepository(LocalContext.current) | ||
|
|
There was a problem hiding this comment.
NotableNavHost creates a new AppRepository(LocalContext.current) on every recomposition and passes an Activity context. This is unnecessary work and can accidentally retain an Activity. Use remember { AppRepository(LocalContext.current.applicationContext) } (or inject via Hilt) so the repository is stable and uses an application context.
| fun requestPermissions(context: Context) { | ||
| if (Build.VERSION.SDK_INT < Build.VERSION_CODES.R) { | ||
| if (ContextCompat.checkSelfPermission( | ||
| context, | ||
| Manifest.permission.WRITE_EXTERNAL_STORAGE | ||
| ) != PackageManager.PERMISSION_GRANTED | ||
| ) { | ||
| ActivityCompat.requestPermissions( | ||
| context as Activity, | ||
| arrayOf(Manifest.permission.WRITE_EXTERNAL_STORAGE), | ||
| 1001 | ||
| ) |
There was a problem hiding this comment.
requestPermissions() casts context to Activity when requesting WRITE_EXTERNAL_STORAGE. If the composable passes a non-Activity context (e.g., a ContextWrapper), this will crash. Consider passing an Activity explicitly, using Context.findActivity() helper, or switching to the Activity Result APIs for permission requests.
|
It's good enough for now, the next step will be to migrate fully to Hilt. Rest of the issues will be addressed in next PR. |
No description provided.