Skip to content

Data loader strategies do not need to be request scoped#3648

Open
CarsonF wants to merge 1 commit intodevelopfrom
data-loader-no-req
Open

Data loader strategies do not need to be request scoped#3648
CarsonF wants to merge 1 commit intodevelopfrom
data-loader-no-req

Conversation

@CarsonF
Copy link
Member

@CarsonF CarsonF commented Feb 13, 2026

Maybe at one point they did need the REQUEST/CONTEXT injected for session/identity.
But now that's passed around as an async context.

@coderabbitai
Copy link

coderabbitai bot commented Feb 13, 2026

📝 Walkthrough

Walkthrough

Removed request-scoped filtering and REQUEST scope usage: the LoaderFactory decorator no longer applies Scope.REQUEST, and loader registry initialization no longer restricts providers to the REQUEST scope.

Changes

Cohort / File(s) Summary
LoaderFactory decorator
src/core/data-loader/loader-factory.decorator.ts
Removed Scope import and replaced Injectable({ scope: Scope.REQUEST })(target) with Injectable()(target) to use the default injectable scope.
Loader registry initialization
src/core/resources/loader.registry.ts
Removed Scope-based runtime filter in provider collection/onModuleInit; providers are now collected without limiting to Scope.REQUEST.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

🚥 Pre-merge checks | ✅ 3 | ❌ 1
❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Description check ⚠️ Warning The description is minimal and lacks structure. It provides only rationale without following the repository's template, which requires Monday task link, structured description section, and ready-for-review checklist. Add Monday task link or explain reason for PR, expand description with details about changes, and include the ready-for-review checklist as specified in the template.
✅ Passed checks (3 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately and concisely describes the main change: removing request scope from data loader strategies, which aligns with the modifications in both decorator files.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Merge Conflict Detection ✅ Passed ✅ No merge conflicts detected when merging into develop

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch data-loader-no-req

No actionable comments were generated in the recent review. 🎉


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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Maybe at one point they did need the REQUEST/CONTEXT injected
for session/identity.
But now that's passed around as an async context.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant