refactor: move mobile modules into domain directory#187
Conversation
First domain move of the module reorganization initiative. Moves 5 mobile-specific modules into src/modules/mobile/ with a barrel export: - mobile-nav.ts - mobile-filters.ts - mobile-filter-logic.ts - mobile-filter-sheet.ts - pull-refresh.ts Re-export shims left at old paths for backwards compatibility with tests and any indirect consumers. script.ts updated to import from the new mobile/ barrel. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
📝 WalkthroughWalkthroughThis PR reorganizes mobile-related modules by moving implementations from Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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 |
|
🚀 Preview deployed: https://537c40d2.megabonk.pages.dev |
🤖 CI Summary
📦 Bundle SizeMain JS: 50KB 📋 What to do if tests fail?
Updated: 2026-04-01T13:51:14.099Z |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/modules/mobile/mobile-filter-logic.ts`:
- Around line 79-84: The Clear All logic sets every HTMLSelectElement to 'all',
which fails for selects like the sortBy controls whose defaults are "name" or
"date_desc" and causes applyFiltersFromSheet() to write empty/invalid values and
countActiveFilters() to treat them as active; update the sheetInputs.forEach
handling in mobile-filter-logic.ts so that for each HTMLSelectElement you either
set it to its known default (e.g., for controls named "sortBy" use "name" or
"date_desc" as appropriate) or only set it to 'all' if that option actually
exists, otherwise skip changing that select; ensure this logic references the
select's name/id to detect sortBy controls and leaves selects unchanged when the
target option is missing so applyFiltersFromSheet() and countActiveFilters()
receive valid values.
In `@src/modules/mobile/mobile-filters.ts`:
- Around line 282-291: The filter toggle's aria-expanded is only toggled in the
click handler; update both showFilterSheet() and hideFilterSheet() to explicitly
set the trigger button's aria-expanded to "true" when opening and "false" when
closing (find the same trigger element used by the click handler—the button
whose handler sets aria-expanded currently), so that
showFilterSheet()/hideFilterSheet() (and the other close paths that call
hideFilterSheet()) keep the trigger state consistent; also apply the same
aria-expanded-fix where showFilterSheet/hideFilterSheet are duplicated (the
other block referenced around lines 348-350).
In `@src/modules/mobile/mobile-nav.ts`:
- Around line 441-444: The subscribe('currentTab', ...) call in initMobileNav is
leaking because its unsubscribe function is discarded; change initMobileNav to
store the returned unsubscribe (e.g., module-level variable like
unsubscribeCurrentTab) and call it before re-subscribing or in a new exported
cleanupMobileNav() function; ensure updateMobileNavState remains the callback
and that cleanupMobileNav() (or calling the stored unsubscribe) is invoked when
tearing down or before re-initializing to avoid accumulated subscriptions.
In `@src/modules/mobile/pull-refresh.ts`:
- Around line 327-332: The review points out that assigning initPullRefresh and
cleanupPullRefresh onto globalThis unintentionally expands the public API
surface; either remove the globalThis assignment or make the export explicit and
documented. Locate the global assignment block that references globalThis and
the functions initPullRefresh and cleanupPullRefresh and: if these functions are
not meant for external consumers, delete the Object.assign(globalThis, {...})
line and rely on module exports only; if they are intended to be public, keep
the assignment but add a clear stability/version doc comment above it (e.g.,
marking it experimental/semver-stable and linking to docs), or gate the
assignment behind a feature flag (e.g., a deliberate opt-in config) so the
global export is intentional and documented.
- Around line 234-272: The call to loadAllData from triggerRefresh can run
concurrently with other callers; add concurrency protection inside the
loadAllData implementation by introducing a shared in-flight guard (e.g., an
isLoading boolean or a single Promise cache) in the data-service's loadAllData
function so that if a load is already running it either returns the existing
Promise or exits early; ensure the guard is set at the start of loadAllData,
cleared on both success and failure, and that callers like triggerRefresh
continue to await the returned Promise so state updates are serialized.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 97255663-e75c-4f15-8075-56e0e1d8168e
📒 Files selected for processing (12)
src/modules/mobile-filter-logic.tssrc/modules/mobile-filter-sheet.tssrc/modules/mobile-filters.tssrc/modules/mobile-nav.tssrc/modules/mobile/index.tssrc/modules/mobile/mobile-filter-logic.tssrc/modules/mobile/mobile-filter-sheet.tssrc/modules/mobile/mobile-filters.tssrc/modules/mobile/mobile-nav.tssrc/modules/mobile/pull-refresh.tssrc/modules/pull-refresh.tssrc/script.ts
| sheetInputs.forEach(sheetInput => { | ||
| if (sheetInput instanceof HTMLInputElement && sheetInput.type === 'checkbox') { | ||
| sheetInput.checked = false; | ||
| } else if (sheetInput instanceof HTMLSelectElement) { | ||
| sheetInput.value = 'all'; | ||
| } |
There was a problem hiding this comment.
Don't reset every select to all.
Line 83 assumes every sheet select has an "all" option, but src/modules/mobile/mobile-filters.ts:76-83 and src/modules/mobile/mobile-filters.ts:207-212 define sortBy controls whose defaults are "name" and "date_desc". Hitting Clear All on those tabs leaves the sheet select with no matching option, and applyFiltersFromSheet() then writes an empty value back into the main filters. That empty value is also treated as active by countActiveFilters().
Suggested fix
sheetInputs.forEach(sheetInput => {
if (sheetInput instanceof HTMLInputElement && sheetInput.type === 'checkbox') {
sheetInput.checked = false;
} else if (sheetInput instanceof HTMLSelectElement) {
- sheetInput.value = 'all';
+ sheetInput.value = sheetInput.options[0]?.value ?? '';
}
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| sheetInputs.forEach(sheetInput => { | |
| if (sheetInput instanceof HTMLInputElement && sheetInput.type === 'checkbox') { | |
| sheetInput.checked = false; | |
| } else if (sheetInput instanceof HTMLSelectElement) { | |
| sheetInput.value = 'all'; | |
| } | |
| sheetInputs.forEach(sheetInput => { | |
| if (sheetInput instanceof HTMLInputElement && sheetInput.type === 'checkbox') { | |
| sheetInput.checked = false; | |
| } else if (sheetInput instanceof HTMLSelectElement) { | |
| sheetInput.value = sheetInput.options[0]?.value ?? ''; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/modules/mobile/mobile-filter-logic.ts` around lines 79 - 84, The Clear
All logic sets every HTMLSelectElement to 'all', which fails for selects like
the sortBy controls whose defaults are "name" or "date_desc" and causes
applyFiltersFromSheet() to write empty/invalid values and countActiveFilters()
to treat them as active; update the sheetInputs.forEach handling in
mobile-filter-logic.ts so that for each HTMLSelectElement you either set it to
its known default (e.g., for controls named "sortBy" use "name" or "date_desc"
as appropriate) or only set it to 'all' if that option actually exists,
otherwise skip changing that select; ensure this logic references the select's
name/id to detect sortBy controls and leaves selects unchanged when the target
option is missing so applyFiltersFromSheet() and countActiveFilters() receive
valid values.
| export function hideFilterSheet(): void { | ||
| const sheet = safeGetElementById('filter-bottom-sheet'); | ||
| if (!sheet) return; | ||
|
|
||
| isSheetOpen = false; | ||
| sheet.classList.remove('active'); | ||
| document.body.classList.remove('filter-sheet-open'); | ||
|
|
||
| document.removeEventListener('keydown', _handleKeyboardNavigation); | ||
| document.removeEventListener('keydown', _handleFocusTrap); |
There was a problem hiding this comment.
Keep the trigger's aria-expanded state in showFilterSheet()/hideFilterSheet().
aria-expanded is only updated in the trigger's click handler right now. Backdrop, close button, Apply, and Escape all close the sheet through hideFilterSheet() (src/modules/mobile/mobile-filter-sheet.ts:97-111 and src/modules/mobile/mobile-filter-sheet.ts:121-123), so the Filters button can keep announcing itself as expanded after the dialog is already closed.
Suggested fix
export function showFilterSheet(): void {
previouslyFocusedElement = document.activeElement as HTMLElement;
@@
isSheetOpen = true;
+ safeQuerySelector('.mobile-filter-btn')?.setAttribute('aria-expanded', 'true');
sheet.classList.add('active');
document.body.classList.add('filter-sheet-open');
@@
export function hideFilterSheet(): void {
const sheet = safeGetElementById('filter-bottom-sheet');
if (!sheet) return;
isSheetOpen = false;
+ safeQuerySelector('.mobile-filter-btn')?.setAttribute('aria-expanded', 'false');
sheet.classList.remove('active');
document.body.classList.remove('filter-sheet-open');
@@
btn.addEventListener('click', () => {
toggleFilterSheet();
- btn.setAttribute('aria-expanded', isSheetOpen ? 'true' : 'false');
});Also applies to: 348-350
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/modules/mobile/mobile-filters.ts` around lines 282 - 291, The filter
toggle's aria-expanded is only toggled in the click handler; update both
showFilterSheet() and hideFilterSheet() to explicitly set the trigger button's
aria-expanded to "true" when opening and "false" when closing (find the same
trigger element used by the click handler—the button whose handler sets
aria-expanded currently), so that showFilterSheet()/hideFilterSheet() (and the
other close paths that call hideFilterSheet()) keep the trigger state
consistent; also apply the same aria-expanded-fix where
showFilterSheet/hideFilterSheet are duplicated (the other block referenced
around lines 348-350).
| // Subscribe to tab changes to update nav state | ||
| subscribe('currentTab', newTab => { | ||
| updateMobileNavState(newTab as string); | ||
| }); |
There was a problem hiding this comment.
Memory leak: subscribe() return value is discarded.
The subscribe('currentTab', ...) call returns an unsubscribe function that's never stored. If initMobileNav() is called multiple times (e.g., during hot module replacement or test setup), subscriptions accumulate and callbacks persist indefinitely.
Proposed fix: Store and call unsubscribe on cleanup
Add module-level unsubscribe tracking:
+let unsubscribeCurrentTab: (() => void) | null = null;
+
export function initMobileNav(): void {
const mobileNav = safeQuerySelector('.mobile-bottom-nav');
// ...
// Subscribe to tab changes to update nav state
- subscribe('currentTab', newTab => {
+ // Clean up any existing subscription first
+ unsubscribeCurrentTab?.();
+ unsubscribeCurrentTab = subscribe('currentTab', newTab => {
updateMobileNavState(newTab as string);
});Optionally, export a cleanupMobileNav() function for symmetry with other mobile modules.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Subscribe to tab changes to update nav state | |
| subscribe('currentTab', newTab => { | |
| updateMobileNavState(newTab as string); | |
| }); | |
| let unsubscribeCurrentTab: (() => void) | null = null; | |
| export function initMobileNav(): void { | |
| const mobileNav = safeQuerySelector('.mobile-bottom-nav'); | |
| // ... other initialization code ... | |
| // Subscribe to tab changes to update nav state | |
| // Clean up any existing subscription first | |
| unsubscribeCurrentTab?.(); | |
| unsubscribeCurrentTab = subscribe('currentTab', newTab => { | |
| updateMobileNavState(newTab as string); | |
| }); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/modules/mobile/mobile-nav.ts` around lines 441 - 444, The
subscribe('currentTab', ...) call in initMobileNav is leaking because its
unsubscribe function is discarded; change initMobileNav to store the returned
unsubscribe (e.g., module-level variable like unsubscribeCurrentTab) and call it
before re-subscribing or in a new exported cleanupMobileNav() function; ensure
updateMobileNavState remains the callback and that cleanupMobileNav() (or
calling the stored unsubscribe) is invoked when tearing down or before
re-initializing to avoid accumulated subscriptions.
| async function triggerRefresh(): Promise<void> { | ||
| state.isRefreshing = true; | ||
| updateIndicator(PULL_REFRESH_CONFIG.PULL_THRESHOLD, true); | ||
|
|
||
| logger.info({ | ||
| operation: 'pull-refresh.triggered', | ||
| data: { source: 'touch-gesture' }, | ||
| }); | ||
|
|
||
| const startTime = performance.now(); | ||
|
|
||
| try { | ||
| await loadAllData(); | ||
|
|
||
| const duration = Math.round(performance.now() - startTime); | ||
| logger.info({ | ||
| operation: 'pull-refresh.complete', | ||
| durationMs: duration, | ||
| success: true, | ||
| }); | ||
|
|
||
| ToastManager.success('Data refreshed!'); | ||
| } catch (error) { | ||
| const err = error as Error; | ||
| logger.error({ | ||
| operation: 'pull-refresh.failed', | ||
| error: { | ||
| name: err.name, | ||
| message: err.message, | ||
| module: 'pull-refresh', | ||
| }, | ||
| }); | ||
| ToastManager.error('Failed to refresh data'); | ||
| } finally { | ||
| state.isRefreshing = false; | ||
| state.lastRefreshTime = Date.now(); | ||
| resetIndicator(); | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for any mutex, lock, or in-progress flag in loadAllData
rg -n -B5 -A20 'export async function loadAllData' --type=tsRepository: jtn0123/MegaBonk
Length of output: 1885
🏁 Script executed:
# Get the full loadAllData function to check for concurrency guards
sed -n '374,534p' src/modules/data-service.tsRepository: jtn0123/MegaBonk
Length of output: 6886
🏁 Script executed:
# Find all calls to loadAllData to understand call patterns
rg -n 'loadAllData\(' --type=ts -B2 -A2 | head -80Repository: jtn0123/MegaBonk
Length of output: 4373
🏁 Script executed:
# Find setState implementation to check for concurrent call safety
rg -n 'function setState|export.*setState' --type=ts -A10 | head -60Repository: jtn0123/MegaBonk
Length of output: 2002
Race condition: loadAllData() lacks concurrent call protection.
The triggerRefresh() function calls loadAllData() without checking if another load is already in progress. The function can be invoked from multiple sources: initial page load (script.ts), offline UI retry (offline-ui.ts), pull-to-refresh (pull-refresh.ts), and error recovery (events-core.ts).
Since loadAllData() has no mutex or in-progress flag, concurrent calls can interleave setState() operations, corrupting app state. The local isRefreshing flag only prevents double-triggers within pull-refresh itself, not cross-module concurrent calls.
Add a guard in data-service.ts to prevent concurrent loads—either check a shared isLoading flag before proceeding, or return the existing promise if a load is already in flight.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/modules/mobile/pull-refresh.ts` around lines 234 - 272, The call to
loadAllData from triggerRefresh can run concurrently with other callers; add
concurrency protection inside the loadAllData implementation by introducing a
shared in-flight guard (e.g., an isLoading boolean or a single Promise cache) in
the data-service's loadAllData function so that if a load is already running it
either returns the existing Promise or exits early; ensure the guard is set at
the start of loadAllData, cleared on both success and failure, and that callers
like triggerRefresh continue to await the returned Promise so state updates are
serialized.
| if (typeof globalThis !== 'undefined') { | ||
| Object.assign(globalThis, { | ||
| initPullRefresh, | ||
| cleanupPullRefresh, | ||
| }); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider documenting API stability for globalThis exports.
Assigning initPullRefresh and cleanupPullRefresh to globalThis exposes them for external scripts, but there's no versioning or stability contract. If these are intended for external consumption, consider documenting them; otherwise, consider removing the global assignment to reduce API surface.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/modules/mobile/pull-refresh.ts` around lines 327 - 332, The review points
out that assigning initPullRefresh and cleanupPullRefresh onto globalThis
unintentionally expands the public API surface; either remove the globalThis
assignment or make the export explicit and documented. Locate the global
assignment block that references globalThis and the functions initPullRefresh
and cleanupPullRefresh and: if these functions are not meant for external
consumers, delete the Object.assign(globalThis, {...}) line and rely on module
exports only; if they are intended to be public, keep the assignment but add a
clear stability/version doc comment above it (e.g., marking it
experimental/semver-stable and linking to docs), or gate the assignment behind a
feature flag (e.g., a deliberate opt-in config) so the global export is
intentional and documented.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
❌ SonarCloud Analysis
Quality Gate: Failed New Code
Ratings
Files Changed
Issues (1 found)🧹 1 Code Smell
Overall Project
View full analysis on SonarCloud SonarCloud · |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| const isMoreTab = MORE_TAB_NAMES.has(currentTab); | ||
| const currentMoreTab = isMoreTab ? MORE_MENU_TABS.find(t => t.tab === currentTab) : null; | ||
|
|
||
| navItems.forEach(item => { |
There was a problem hiding this comment.
🧹 SonarCloud CRITICAL: Refactor this function to reduce its Cognitive Complexity from 19 to the 15 allowed.
|
This PR has been automatically marked as stale because it has not had recent activity. It will be closed in 7 days if no further activity occurs. |
1 participant
Summary
src/modules/mobile/with barrel exportscript.tsupdated to import frommobile/index.tsbarrelFiles moved
mobile-nav.ts→mobile/mobile-nav.tsmobile-filters.ts→mobile/mobile-filters.tsmobile-filter-logic.ts→mobile/mobile-filter-logic.tsmobile-filter-sheet.ts→mobile/mobile-filter-sheet.tspull-refresh.ts→mobile/pull-refresh.tsTest plan
npm run lint— 0 errorsnpm run build— succeeds, bundle size unchangednpx vitest run— 244 test files, 10,034 tests passPart of JTN-128 (Phase 2: Reorganize modules into domain directories)
🤖 Generated with Claude Code
Summary by CodeRabbit
mobile/subdirectory for improved code organization and maintainability.