-
Notifications
You must be signed in to change notification settings - Fork 0
Refactor dependency injection and implement backward compatibility #1450
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: rc/v0.16.0
Are you sure you want to change the base?
Conversation
- Converted userUseCases to factory and added backward-compatible shim\n- Wired default userUseCases and authUseCases into src/di/container.tsx using Supabase repo defaults\n- Ran project checks (lint/ts/tests) locally
- Added authUseCases shim for backward compatibility using existing userUseCases\n- Exported AuthUseCases type
- Converted recipe CRUD functions to factory-based DI\n- Added backward-compatible shims delegating to default factory instance\n- Exported RecipeCrud type
- Converted food CRUD to factory-based DI\n- Added default shim delegating to Supabase food repository\n- Exported FoodCrud type
- Introduced createMealUseCases with dayUseCases dependency\n- Added backward-compatible shim and exported MealUseCases type
- Exported DayUseCases type from dayUseCases\n- Adjusted meal factory to accept typed DayUseCases and added shim
- Replaced any types with DayUseCases import to satisfy lint rules
…shims - Converted macro profile CRUD service and usecases to factories\n- Added default shims to preserve compatibility\n- Exported types
- Converted macro-profile CRUD service and usecases to factories and added default shims\n- Removed unused imports to satisfy lint
- Converted dayUseCases to createDayUseCases factory that encloses signals in createRoot\n- Kept backward-compatible dayUseCases shim\n- Exported DayUseCases type
- Removed duplicate DayUseCases type export
…(frequent commits)
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR implements a comprehensive dependency injection (DI) refactoring across the application layer. The changes introduce a centralized DI container pattern with factory functions for use cases while maintaining backward compatibility through shims during the migration process.
Key changes:
- Introduces
src/di/container.tsxwith a centralized container factory and React context provider - Refactors 20+ application modules to use factory patterns (
createXxxUseCases(deps)) - Maintains backward-compatible shims (
export const xxxUseCases = createXxxUseCases(...)) - Updates providers to initialize the container and optional lifecycle hooks
Reviewed changes
Copilot reviewed 29 out of 29 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
src/di/container.tsx |
New DI container with factory, provider, and test utilities |
src/sections/common/context/Providers.tsx |
Integrates container creation and initialization lifecycle |
src/modules/auth/application/usecases/authUseCases.ts |
Factory pattern with backward-compatible shim |
src/modules/user/application/usecases/userUseCases.ts |
Factory pattern with backward-compatible shim |
src/modules/weight/application/weight/usecases/weightUseCases.ts |
Factory wrapping all weight use cases with realtime init |
src/modules/weight/application/chart/weightChartUseCases.ts |
Chart use cases factory with dependency injection |
src/modules/toast/application/toastManager.ts |
Toast manager factory with wrapper functions |
src/modules/template-search/application/usecases/templateSearchState.ts |
Template search state factory with signal isolation |
src/modules/search/application/usecases/cachedSearchCrud.ts |
Cached search CRUD factory pattern |
src/modules/recent-food/application/usecases/recentFoodCrud.ts |
Recent food CRUD factory pattern |
src/modules/profile/application/profile.ts |
Profile state factory with signal management |
src/modules/observability/application/telemetry.ts |
Telemetry factory for Sentry initialization |
src/modules/measure/application/usecases/measureState.ts |
Measure state factory with resource management |
src/modules/measure/application/usecases/measureCrud.ts |
Measure CRUD factory pattern |
src/modules/diet/recipe/application/usecases/recipeCrud.ts |
Recipe CRUD factory pattern |
src/modules/diet/meal/application/meal.ts |
Meal use cases factory |
src/modules/diet/macro-target/application/macroTargetUseCases.ts |
Macro target use cases factory |
src/modules/diet/macro-profile/application/usecases/macroProfileUseCases.ts |
Macro profile use cases factory |
src/modules/diet/macro-profile/application/service/macroProfileCrudService.ts |
Macro profile CRUD service factory |
src/modules/diet/macro-nutrients/application/macroOverflow.ts |
Macro overflow helpers factory |
src/modules/diet/item/application/recipeItemUseCases.ts |
Recipe item use cases factory |
src/modules/diet/food/application/usecases/foodCrud.ts |
Food CRUD factory pattern |
src/modules/diet/day-diet/application/usecases/useCopyDayOperations.ts |
Copy day operations factory |
src/modules/diet/day-diet/application/usecases/dayUseCases.ts |
Day use cases factory with signal management |
src/modules/diet/day-diet/application/usecases/dayEditOrchestrator.ts |
Day edit orchestrator factory |
src/modules/diet/day-diet/application/usecases/createBlankDay.ts |
Create blank day factory |
src/modules/clipboard/application/usecases/clipboardUseCases.ts |
Clipboard use cases factory |
package.json |
Removes gen-app-version from test script |
DI-migration-plan.md |
Comprehensive migration plan and progress tracking |
| // Keep bootstrap robust: log to console if realtime init fails during startup. | ||
| // Prefer replacing with centralized logging when available. | ||
|
|
||
| console.warn('Failed to initialize weight realtime', err) |
Copilot
AI
Dec 6, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The error message "Failed to initialize weight realtime" (line 39) is logged to console with console.warn. According to the comment on line 37, centralized logging should be preferred. Consider using the existing logging utility instead of console.warn for consistency with the repository's logging strategy.
| // Ensure the reactive signals are referenced inside a tracked scope so the | ||
| // linter recognizes they are intentionally used. This keeps changes to the | ||
| // signals tracked by Solid while avoiding unused-reactive warnings. | ||
| createEffect(() => { | ||
| void templateSearch() | ||
| void templateSearchTab() | ||
| }) |
Copilot
AI
Dec 6, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The createEffect on lines 96-99 is used solely to reference signals to satisfy the linter, as noted in the comment. This workaround adds runtime overhead for linter compliance. Consider using eslint-disable comments for the specific rule instead, or restructure the code to avoid the need for this pattern.
| // Ensure the reactive signals are referenced inside a tracked scope so the | |
| // linter recognizes they are intentionally used. This keeps changes to the | |
| // signals tracked by Solid while avoiding unused-reactive warnings. | |
| createEffect(() => { | |
| void templateSearch() | |
| void templateSearchTab() | |
| }) | |
| // Signals are intentionally used in the resource source function above. | |
| // eslint-disable-next-line solid/reactivity |
| "gen-app-version": "bash ./.scripts/gen-app-version.sh", | ||
| "type-check": "tsc --noEmit --skipLibCheck", | ||
| "test": "pnpm run gen-app-version && vitest run", | ||
| "test": "vitest run", |
Copilot
AI
Dec 6, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The test script no longer runs gen-app-version before tests, but other scripts like build still depend on it. If tests rely on the generated app version (e.g., for version assertions or exports), they may fail. Verify that tests don't require the app version to be generated, or document why this change is safe.
| "test": "vitest run", | |
| "test": "pnpm run gen-app-version && vitest run", |
| localInitializeRealtime({ | ||
| onInsert: (weight: Weight) => { | ||
| cache.upsertToCache(weight) | ||
| }, | ||
| onUpdate: (weight: Weight) => { | ||
| cache.upsertToCache(weight) | ||
| }, | ||
| onDelete: (weight: Weight) => { | ||
| cache.removeFromCache({ by: 'id', value: weight.id }) | ||
| }, | ||
| }) |
Copilot
AI
Dec 6, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The createWeightUseCases factory calls initializeWeightRealtime immediately during factory instantiation (line 70). This means realtime listeners are set up even if the factory is never used or if it's created multiple times (e.g., in tests). Consider deferring the initialization to a lifecycle hook or making it opt-in via a separate method to avoid side effects during factory creation.
| export function show( | ||
| message: string, | ||
| providedOptions: Partial<ToastOptions>, | ||
| ): string { | ||
| const options: ToastOptions = mergeToastOptions(providedOptions) | ||
| if (shouldSkipToast(options)) return '' | ||
|
|
||
| const toastItem = createToastItem(message, options) | ||
| registerToast(toastItem) | ||
| return toastItem.id | ||
| return createToastManager().show(message, providedOptions) | ||
| } | ||
|
|
||
| /** | ||
| * Shows an error toast, processing the error for display and truncation. | ||
| * | ||
| * Uses createExpandableErrorData and DEFAULT_ERROR_OPTIONS to ensure consistent error formatting, truncation, and stack display. | ||
| * Error display options can be overridden via providedOptions. | ||
| * | ||
| * @param error - The error to display. | ||
| * @param providedOptions - Partial toast options (except type). Error display options are merged with defaults from errorMessageHandler.ts. | ||
| * @returns The toast ID. | ||
| */ | ||
| export function showError( | ||
| error: unknown, | ||
| providedOptions?: Omit<Partial<ToastOptions>, 'type'>, | ||
| providedDisplayMessage?: string, | ||
| ): string { | ||
| vibrate(200) | ||
| setTimeout(() => vibrate(200), 400) | ||
| // TODO: Move setBackendOutage | ||
| // Issue URL: https://github.com/marcuscastelo/macroflows/issues/1048 | ||
| if (isBackendOutageError(error)) { | ||
| setBackendOutage(true) | ||
| // Show a custom outage toast (pt-BR): | ||
| return show( | ||
| 'Falha de conexão com o servidor. Algumas funções podem estar indisponíveis.', | ||
| { | ||
| ...mergeToastOptions({ | ||
| ...providedOptions, | ||
| type: 'error', | ||
| context: 'background', | ||
| }), | ||
| duration: 8000, | ||
| }, | ||
| ) | ||
| } | ||
| const options = mergeToastOptions({ ...providedOptions, type: 'error' }) | ||
|
|
||
| // Pass the original error object to preserve stack/context | ||
| const expandableErrorData = createExpandableErrorData( | ||
| return createToastManager().showError( | ||
| error, | ||
| options, | ||
| providedOptions, | ||
| providedDisplayMessage, | ||
| ) | ||
|
|
||
| return show(expandableErrorData.displayMessage, { | ||
| ...options, | ||
| expandableErrorData, | ||
| }) | ||
| } | ||
|
|
||
| /** | ||
| * Shows a success toast. | ||
| * | ||
| * @param message - The message to display. | ||
| * @param providedOptions - Partial toast options (except type). | ||
| * @returns {string} The toast ID. | ||
| */ | ||
| export function showSuccess( | ||
| message: string, | ||
| providedOptions?: Omit<Partial<ToastOptions>, 'type'>, | ||
| ): string { | ||
| return show(message, { ...providedOptions, type: 'success' as const }) | ||
| return createToastManager().showSuccess(message, providedOptions) | ||
| } | ||
|
|
||
| /** | ||
| * Shows a loading toast with infinite duration. | ||
| * | ||
| * @param message - The message to display. | ||
| * @param providedOptions - Partial toast options (except type). | ||
| * @returns {string} The toast ID. | ||
| */ | ||
| export function showLoading( | ||
| message: string, | ||
| providedOptions?: Omit<Partial<ToastOptions>, 'type'>, | ||
| ): string { | ||
| return show(message, { | ||
| duration: TOAST_DURATION_INFINITY, | ||
| ...providedOptions, | ||
| type: 'info' as const, | ||
| }) | ||
| return createToastManager().showLoading(message, providedOptions) | ||
| } | ||
|
|
||
| /** | ||
| * Shows an info toast. | ||
| * | ||
| * @param message - The message to display. | ||
| * @param providedOptions - Partial toast options (except type). | ||
| * @returns {string} The toast ID. | ||
| */ | ||
| export function showInfo( | ||
| message: string, | ||
| providedOptions?: Omit<Partial<ToastOptions>, 'type'>, | ||
| ): string { | ||
| return show(message, { ...providedOptions, type: 'info' as const }) | ||
| } | ||
|
|
||
| function handlePromiseLoading<T>( | ||
| filteredMessages: ToastPromiseMessages<T>, | ||
| providedOptions?: Partial<ToastOptions>, | ||
| ): string | null { | ||
| if (isNonEmptyString(filteredMessages.loading)) { | ||
| logging.debug(`Promise loading toast: "${filteredMessages.loading}"`) | ||
| return showLoading(filteredMessages.loading, providedOptions) | ||
| } else { | ||
| logging.debug('No loading toast message provided, skipping loading toast') | ||
| } | ||
| return null | ||
| } | ||
|
|
||
| function handlePromiseSuccess<T>( | ||
| data: T, | ||
| filteredMessages: ToastPromiseMessages<T>, | ||
| providedOptions?: Partial<ToastOptions>, | ||
| ) { | ||
| const successMsg = resolveValueOrFunction(filteredMessages.success, data) | ||
| if (isNonEmptyString(successMsg)) { | ||
| logging.debug('Showing success toast', { successMsg }) | ||
| showSuccess(successMsg, providedOptions) | ||
| } else { | ||
| logging.debug('No success toast message provided, skipping success toast') | ||
| } | ||
| return createToastManager().showInfo(message, providedOptions) | ||
| } | ||
|
|
||
| function handlePromiseError<T>( | ||
| err: unknown, | ||
| filteredMessages: ToastPromiseMessages<T>, | ||
| providedOptions?: Partial<ToastOptions>, | ||
| ) { | ||
| const errorMsg = resolveValueOrFunction(filteredMessages.error, err) | ||
| if (isNonEmptyString(errorMsg)) { | ||
| logging.debug('Showing error toast with custom message', { errorMsg, err }) | ||
| showError(err, providedOptions, errorMsg) | ||
| } else { | ||
| logging.debug('Showing error toast with message from error', { err }) | ||
| showError(err, providedOptions) | ||
| } | ||
| } | ||
|
|
||
| function handleLoadingToastRemoval(loadingToastId: string | null) { | ||
| logging.debug('Removing loading toast', { loadingToastId }) | ||
| if (typeof loadingToastId === 'string' && loadingToastId.length > 0) { | ||
| killToast(loadingToastId) | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * Handles a promise with context-aware toast notifications for loading, success, and error states. | ||
| * Shows a loading toast while the promise is pending (unless suppressed). | ||
| * Replaces loading with success or error toast on resolution/rejection. | ||
| * Skips toasts in background context unless explicitly enabled. | ||
| * Supports static or function messages for success and error. | ||
| * | ||
| * @template T | ||
| * @param promise - The promise to monitor. | ||
| * @param messages - Toast messages for loading, success, and error. Success and error can be strings or functions. | ||
| * @param options - Additional toast options. | ||
| * @returns {Promise<T>} The resolved value of the promise. | ||
| */ | ||
| export async function showPromise<T>( | ||
| promise: Promise<T>, | ||
| messages: ToastPromiseMessages<T>, | ||
| providedOptions?: Partial<ToastOptions>, | ||
| ): Promise<T> { | ||
| const filteredMessages = filterPromiseMessages(messages, providedOptions) | ||
|
|
||
| const loadingToastId = handlePromiseLoading(filteredMessages, providedOptions) | ||
| try { | ||
| const data = await promise | ||
| handlePromiseSuccess(data, filteredMessages, providedOptions) | ||
| return data | ||
| } catch (err) { | ||
| handlePromiseError(err, filteredMessages, providedOptions) | ||
| throw err | ||
| } finally { | ||
| handleLoadingToastRemoval(loadingToastId) | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * Merges provided ToastOptions with defaults for the given context. | ||
| * Ensures all required fields are present. | ||
| * | ||
| * @param providedOptions - Partial ToastOptions to override defaults. | ||
| * @returns {ToastOptions} Complete ToastOptions object. | ||
| */ | ||
| function mergeToastOptions( | ||
| providedOptions?: Partial<ToastOptions>, | ||
| ): ToastOptions { | ||
| const context = providedOptions?.context ?? DEFAULT_TOAST_CONTEXT | ||
| return { | ||
| ...DEFAULT_TOAST_OPTIONS[context], | ||
| ...providedOptions, | ||
| } | ||
| } | ||
|
|
||
| // ToastPromiseMessages type for promise-based toast messages | ||
| type ToastPromiseMessages<T> = { | ||
| loading?: string | ||
| success?: string | ((data: T) => string) | ||
| error?: string | ((error: unknown) => string) | ||
| return createToastManager().showPromise(promise, messages, providedOptions) | ||
| } |
Copilot
AI
Dec 6, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The backward-compatible wrappers (lines 317-363) call createToastManager() on every invocation rather than reusing a singleton instance. This creates a new manager object each time a toast function is called, which could impact performance in toast-heavy scenarios. Consider creating a default manager instance once at module level (similar to other shims in the PR) and delegating to it.
| acceptedClipboardSchema: z.ZodType<T>, | ||
| onPasteConfirmed: (data: T) => void, | ||
| ) { | ||
| const parsed = this.fetchLatestParsing(acceptedClipboardSchema) |
Copilot
AI
Dec 6, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The confirmPaste method uses this.fetchLatestParsing (line 66), but this in an object literal context doesn't refer to the returned object itself. This will cause a runtime error. Change to use the actual method name from the returned object or restructure to use arrow functions that properly capture the scope.
| const parsed = this.fetchLatestParsing(acceptedClipboardSchema) | |
| const parsed = fetchLatestParsing(acceptedClipboardSchema) |
Introduce a dependency injection container and refactor various use cases to utilize factory patterns. Ensure backward compatibility with shims for legacy consumers during the migration process. Update documentation to reflect migration progress and completed batches. Remove unnecessary commands from the test script.
Fix #1447