Skip to content

Conversation

@nielsenko
Copy link
Collaborator

@nielsenko nielsenko commented Oct 31, 2025

Description

Adds a convenience method injectAt() to Router for injecting injectables at a specific path. This is a shorthand for calling group(path).inject(injectable).

Related Issues

  • Fixes: #?

Pre-Launch Checklist

  • This update focuses on a single feature or bug fix.
  • I have read and followed the Dart Style Guide and formatted the code using dart format.
  • I have referenced at least one issue this PR fixes or is related to.
  • I have updated/added relevant documentation (doc comments with ///), ensuring consistency with existing project documentation.
  • I have added new tests to verify the changes.
  • All existing and new tests pass successfully.
  • I have documented any breaking changes below.

Breaking Changes

  • Includes breaking changes.
  • No breaking changes.

Additional Notes

None.

Summary by CodeRabbit

  • New Features

    • Router now supports injectAt() for mounting handlers at specific base paths, enabling more flexible nested routing structures and modular application architecture.
  • Tests

    • New test coverage validates path-based handler mounting and nested routing behavior across different path configurations.

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Oct 31, 2025

📝 Walkthrough

Walkthrough

A new public method injectAt(String path, InjectableIn<Router<T>> injectable) is added to the Router API. This method groups a path and injects an injectable into the resulting sub-router in a single operation. Corresponding test coverage validates the functionality with a HandlerObject mounted at a nested path.

Changes

Cohort / File(s) Summary
Router extension API
lib/src/router/router.dart
Adds injectAt method as extension on Router<T> that combines group(path) and inject(injectable) operations for path-scoped injection
Test coverage
test/router/router_inject_test.dart
Adds test group exercising injectAt API with _EchoHandlerObject mounted at nested path /api/echo, validating 200 response and request handling

Sequence Diagram(s)

sequenceDiagram
    participant Client
    participant Router
    participant Sub-Router
    participant Handler

    Client->>Router: injectAt('/api', handler)
    Router->>Router: group('/api')
    activate Router
    Note over Router: Creates or reuses sub-router
    deactivate Router
    Router->>Sub-Router: inject(handler)
    activate Sub-Router
    Note over Sub-Router: Injects handler into sub-router
    deactivate Sub-Router
    
    Client->>Router: GET /api/echo
    Router->>Sub-Router: Route to /echo
    Sub-Router->>Handler: Handle request
    Handler->>Sub-Router: Response
    Sub-Router->>Router: Response
    Router->>Client: 200 OK
Loading

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

  • New method is straightforward composition of existing group() and inject() APIs
  • Single new extension method with no complex logic
  • Test coverage is minimal but validates the expected behavior

Possibly related PRs

  • feat(router): Router.group #157: The injectAt method directly depends on the Router.group() API added in this PR, which is called internally by the new method
  • feat: Introduce HandlerObject #210: This PR introduces the HandlerObject and RouterInjectable injection surface that the new injectAt API builds upon and tests against
  • feat: Introduce MiddlewareObject #211: Related extension of the Router injection API surface, adding RouterInjectable and middleware-related injection patterns that complement the injectAt functionality

Pre-merge checks and finishing touches

❌ Failed checks (1 warning, 1 inconclusive)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
Description Check ❓ Inconclusive The pull request description includes all required sections from the template with proper structure: Description, Related Issues, Pre-Launch Checklist, Breaking Changes, and Additional Notes. However, the Related Issues section contains a placeholder entry "- Fixes: #?" that is not filled in, and the corresponding Pre-Launch Checklist item "I have referenced at least one issue this PR fixes or is related to" was left unchecked. While the description section itself provides a basic explanation of the convenience method, this critical gap suggests the PR description is incomplete and may not be ready for merge. Please complete the Related Issues section by referencing a specific issue number in the format "Closes/Fixes: #<issue_number>" or clearly indicate if this PR is not tied to any tracked issue. Also verify that the corresponding Pre-Launch Checklist item is accurately marked. This will ensure the PR documentation is complete and all requirements are transparently addressed.
✅ Passed checks (1 passed)
Check name Status Explanation
Title Check ✅ Passed The pull request title "feat: Add injectAt extension method on Router" directly and clearly describes the main change in the changeset. It is specific, concise, and conveys that a new extension method is being added to Router with its type parameter. A teammate scanning commit history would immediately understand the primary change from this title.
✨ Finishing touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

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.

@codecov
Copy link

codecov bot commented Oct 31, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 91.48%. Comparing base (a7bba28) to head (c046453).
⚠️ Report is 2 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #261   +/-   ##
=======================================
  Coverage   91.48%   91.48%           
=======================================
  Files          89       89           
  Lines        3475     3477    +2     
  Branches     1769     1770    +1     
=======================================
+ Hits         3179     3181    +2     
  Misses        296      296           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@nielsenko
Copy link
Collaborator Author

@CodeRabbit review

@nielsenko nielsenko self-assigned this Oct 31, 2025
@coderabbitai
Copy link
Contributor

coderabbitai bot commented Oct 31, 2025

✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Nitpick comments (1)
lib/src/router/router.dart (1)

236-238: Consider enhancing the documentation with examples and context.

The implementation is correct and follows the intended pattern of chaining group(path).inject(injectable). However, the doc comment could be more helpful by including an example showing the equivalence to the explicit approach and clarifying behavior (e.g., that it creates a sub-router at the path if one doesn't exist).

For consistency with other methods in this file (e.g., use, attach), consider expanding the documentation:

- /// Injects [injectable] at [path]
+ /// Injects [injectable] at [path].
+ ///
+ /// This is a convenience method equivalent to calling `group(path).inject(injectable)`.
+ /// The [path] string defines the route prefix where the injectable will be mounted.
+ ///
+ /// Example:
+ /// ```dart
+ /// final router = RelicRouter();
+ /// router.injectAt('/api', userModule);
+ /// // Equivalent to:
+ /// // router.group('/api').inject(userModule);
+ /// ```
  void injectAt(final String path, final InjectableIn<Router<T>> injectable) =>
      group(path).inject(injectable);
📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between a7bba28 and c046453.

📒 Files selected for processing (2)
  • lib/src/router/router.dart (1 hunks)
  • test/router/router_inject_test.dart (1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.dart

📄 CodeRabbit inference engine (.github/copilot-instructions.md)

**/*.dart: All Dart code must pass static analysis using dart analyze --fatal-infos with no issues
All Dart files must be formatted with dart format (CI enforces dart format --set-exit-if-changed .)

Files:

  • lib/src/router/router.dart
  • test/router/router_inject_test.dart
lib/**/*.dart

📄 CodeRabbit inference engine (.github/copilot-instructions.md)

lib/**/*.dart: Use Uint8List for request/response bodies for performance; avoid List for body payloads
Use type-safe HTTP header parsing and validation when accessing headers
Use router with trie-based matching and symbol-based path parameters (e.g., #name, #age) for routing
Ensure WebSocket handling includes proper lifecycle management (e.g., ping/pong for connection health)

Files:

  • lib/src/router/router.dart
test/**/*.dart

📄 CodeRabbit inference engine (.github/copilot-instructions.md)

test/**/*.dart: Tests should follow the Given-When-Then pattern in descriptions (flexible structuring allowed)
Use Arrange-Act-Assert pattern within test bodies
Provide clear, descriptive test titles; prefer single responsibility per test unless related assertions improve clarity
Place tests in the test/ directory mirroring the lib/ structure

Files:

  • test/router/router_inject_test.dart
🧠 Learnings (3)
📓 Common learnings
Learnt from: nielsenko
Repo: serverpod/relic PR: 48
File: lib/src/handler/handler.dart:59-67
Timestamp: 2025-04-25T07:39:38.915Z
Learning: Nielsenko prefers using switch statements with pattern matching over if statements when working with sealed classes in Dart, as they provide exhaustiveness checking at compile time and can be more concise.
📚 Learning: 2025-10-09T16:21:09.310Z
Learnt from: CR
Repo: serverpod/relic PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-10-09T16:21:09.310Z
Learning: Applies to lib/**/*.dart : Use router with trie-based matching and symbol-based path parameters (e.g., `#name`, `#age`) for routing

Applied to files:

  • lib/src/router/router.dart
  • test/router/router_inject_test.dart
📚 Learning: 2025-05-05T14:40:00.323Z
Learnt from: nielsenko
Repo: serverpod/relic PR: 52
File: lib/src/router/router.dart:37-53
Timestamp: 2025-05-05T14:40:00.323Z
Learning: In the Router and PathTrie implementation in Dart, both static and dynamic routes consistently throw ArgumentError when attempting to add duplicate routes, despite comments suggesting dynamic routes would be overwritten with a warning.

Applied to files:

  • test/router/router_inject_test.dart
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
  • GitHub Check: Run Unit Tests (macos-latest, beta, downgrade)
  • GitHub Check: Run Unit Tests (macos-latest, beta, upgrade)
  • GitHub Check: Run Unit Tests (macos-latest, stable, upgrade)
  • GitHub Check: Run Unit Tests (macos-latest, 3.7.0, upgrade)
  • GitHub Check: Run Unit Tests (macos-latest, 3.7.0, downgrade)
  • GitHub Check: Run Unit Tests (macos-latest, stable, downgrade)
🔇 Additional comments (1)
test/router/router_inject_test.dart (1)

46-64: LGTM! Test structure and coverage are solid.

The test correctly validates the injectAt functionality:

  • Follows the Given-When-Then pattern as per coding guidelines
  • Uses Arrange-Act-Assert structure in the test body
  • Clear, descriptive test title
  • Properly verifies that injectAt('/api', handler) mounts the handler at the combined path /api/echo

@nielsenko nielsenko marked this pull request as ready for review November 2, 2025 10:40
@nielsenko nielsenko requested a review from a team November 2, 2025 11:20
Copy link
Contributor

@SandPod SandPod left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@nielsenko nielsenko merged commit bba3b8f into serverpod:main Nov 2, 2025
49 of 50 checks passed
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.

2 participants