Skip to content

Conversation

Copy link
Contributor

Copilot AI commented Oct 7, 2025

  • Understand the issue: Add NewContext.withRequest(Request req) convenience method
  • Explore repository structure and understand the current pattern
  • Set up Dart SDK and install dependencies
  • Add withRequest method to NewContext class in lib/src/adapter/context.dart
  • Create comprehensive tests for the new method
  • Restructure tests to follow repository test guidelines
    • Moved shared action to setUp blocks
    • Split combined assertions into separate tests with clear titles
    • Updated group descriptions to include both Given and When clauses
    • Increased test count from 7 to 10 tests with clearer responsibilities
  • Run existing tests to ensure no regressions (3086 tests passed)
  • Format and analyze the code (no issues found)
  • Update copilot-instructions.md with testing guidelines
  • Refine guidelines to be flexible and context-dependent rather than prescriptive

Summary

Refined the testing guidelines in .github/copilot-instructions.md to be more flexible:

  • Changed from strict "must follow" to "should generally follow" language
  • Acknowledged multiple valid approaches depending on context
  • Clarified that structure choices (splitting/combining Given-When-Then) depend on what improves clarity
  • Maintained the example as one valid approach rather than "the" approach
Original prompt

This section details on the original issue you should resolve

<issue_title>Add NewContext.withRequest(Request req) convenience method</issue_title>
<issue_description>Currently, when middleware needs to rewrite a request and pass it to the inner handler, it must use the low-level pattern:

final rewrittenRequest = req.copyWith(requestedUri: newRequested);
return await inner(rewrittenRequest.toContext(ctx.token));

This pattern is too low-level and error-prone. We should add a convenience method on NewContext to simplify this:

NewContext withRequest(Request req)

This would allow the more readable pattern:

final rewrittenRequest = req.copyWith(requestedUri: newRequested);
return await inner(ctx.withRequest(rewrittenRequest));

Context:

<agent_instructions>This should only touch context.dart</agent_instructions>

Comments on the Issue (you are @copilot in this section)

Fixes #193

💬 Share your feedback on Copilot coding agent for the chance to win a $200 gift card! Click here to start the survey.

Summary by CodeRabbit

  • New Features

    • Added a helper to create a new context using a modified request while preserving the existing token. This supports middleware that rewrites requests without disrupting authentication or lifecycle. No breaking changes.
  • Tests

    • Introduced comprehensive tests validating context lifecycle transitions, request rewriting, and consistent token propagation across new, response, hijack, and connect flows.

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Oct 7, 2025

Important

Review skipped

Auto reviews are disabled on this repository.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Note

Other AI code review bot(s) detected

CodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review.

📝 Walkthrough

Walkthrough

Adds a convenience method NewContext.withRequest(Request) to create a new context with a different Request while preserving the token, and introduces unit tests covering request rewriting and subsequent context transitions (respond, hijack, connect) and token propagation.

Changes

Cohort / File(s) Summary
Context adapter API
lib/src/adapter/context.dart
Added method: NewContext withRequest(final Request req) returning NewContext._(req, token); no other logic modified.
Context adapter tests
test/src/adapter/context_test.dart
New tests validating withRequest behavior: preserves token, updates request/requestedUri, and supports transitions to ResponseContext, HijackContext, and ConnectContext independently.

Sequence Diagram(s)

sequenceDiagram
  autonumber
  actor Client
  participant Middleware
  participant InnerHandler
  participant NewContext as NewContext

  Client->>Middleware: HTTP Request
  Middleware->>NewContext: ctx (req, token)
  note over Middleware,NewContext: Rewrite incoming request
  Middleware->>NewContext: ctx.withRequest(rewrittenReq)
  activate NewContext
  NewContext-->>Middleware: newCtx (rewrittenReq, same token)
  deactivate NewContext
  Middleware->>InnerHandler: invoke with newCtx
  InnerHandler-->>Middleware: Response / Transition (respond / hijack / connect)
  Middleware-->>Client: Final outcome
Loading

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Possibly related PRs

  • feat: Router middleware #68 — Also changes context classes in lib/src/adapter/context.dart, introducing a persistent token; directly related as withRequest preserves that token across request rewrites.

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Description Check ⚠️ Warning The pull request description diverges from the repository template by using custom headings (Overview, Problem, Solution) instead of the required “## Description” section, omitting the “## Pre-Launch Checklist” section with its checkboxes, and lacking explicit sections for “## Related Issues” and “## Breaking Changes.” Update the description to follow the template by replacing Overview with “## Description,” adding the “## Pre-Launch Checklist” section with the required checkboxes, including a “## Related Issues” header listing linked issues, and adding a “## Breaking Changes” section even if noting there are none.
✅ Passed checks (4 passed)
Check name Status Explanation
Title Check ✅ Passed The title succinctly summarizes the primary change by indicating the addition of the NewContext.withRequest convenience method without extra noise or irrelevant details, making the main feature clear at a glance.
Linked Issues Check ✅ Passed The changes directly implement the requested NewContext.withRequest(Request req) method from issue #193, preserve the existing token, replace the lower-level toContext(ctx.token) pattern, and include comprehensive tests verifying token propagation, immutability, and context transitions, fully meeting the linked issue’s objectives.
Out of Scope Changes Check ✅ Passed All code modifications are confined to context.dart and its corresponding tests, focusing solely on the addition of the NewContext.withRequest helper and related validation without introducing unrelated or extraneous changes.
Docstring Coverage ✅ Passed No functions found in the changes. Docstring coverage check skipped.

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

Co-authored-by: nielsenko <22237677+nielsenko@users.noreply.github.com>
Copilot AI changed the title [WIP] Add NewContext.withRequest(Request req) convenience method feat: Add NewContext.withRequest convenience method Oct 7, 2025
Copilot AI requested a review from nielsenko October 7, 2025 14:38
Copilot finished work on behalf of nielsenko October 7, 2025 14:38
@codecov
Copy link

codecov bot commented Oct 7, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 91.85%. Comparing base (2910d7a) to head (3451c5c).
⚠️ Report is 1 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #198   +/-   ##
=======================================
  Coverage   91.85%   91.85%           
=======================================
  Files          85       85           
  Lines        3155     3156    +1     
  Branches     1630     1631    +1     
=======================================
+ Hits         2898     2899    +1     
  Misses        257      257           

☔ 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

@CodeRabbit review

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Oct 7, 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.

@nielsenko nielsenko marked this pull request as ready for review October 7, 2025 15:30
@nielsenko
Copy link
Collaborator

@SandPod As I'm listed as participant I cannot merge this myself. This is my first attempt using co-pilot to fix an issue.

@SandPod
Copy link
Contributor

SandPod commented Oct 8, 2025

@nielsenko Yes, an external review is always required. I guess that is still good. I'll take a look!

@nielsenko
Copy link
Collaborator

@nielsenko Yes, an external review is always required. I guess that is still good. I'll take a look!

Yes, only I feel kind of external already, as this was all @copilot .

Copilot AI requested a review from nielsenko October 8, 2025 06:40
Copilot finished work on behalf of nielsenko October 8, 2025 06:40
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.

The groups are not really connected to the tests. I think copilot could probably fix my comments.

Please assert that the tests adhere to our test guidelines.

@nielsenko
Copy link
Collaborator

@copilot Address the feedback.

Co-authored-by: nielsenko <22237677+nielsenko@users.noreply.github.com>
Copy link
Contributor Author

Copilot AI commented Oct 8, 2025

The groups are not really connected to the tests. I think copilot could probably fix my comments.

Please assert that the tests adhere to our test guidelines.

Restructured all tests to follow the repository's test guidelines. Groups now include both Given and When in descriptions, shared actions are in setUp blocks, and each test validates a single requirement. Commit: db1ca2f

@copilot Address the feedback.

Addressed all feedback. Tests now follow the Given-When-Then pattern with shared setup in setUp blocks and separate tests for each requirement. Commit: db1ca2f

Copilot finished work on behalf of nielsenko October 8, 2025 07:02
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.

LGTM! 👍

@nielsenko
Copy link
Collaborator

That went better than I feared 😄
@copilot Please update your instructions regarding the structure of Given-When-Then tests

Co-authored-by: nielsenko <22237677+nielsenko@users.noreply.github.com>
Copy link
Contributor Author

Copilot AI commented Oct 8, 2025

That went better than I feared 😄
@copilot Please update your instructions regarding the structure of Given-When-Then tests

Updated .github/copilot-instructions.md with detailed Given-When-Then test structure guidelines, including group structure, shared setup patterns, single-responsibility tests, and a complete example. Commit: [hash will be added after push]

That went better than I feared 😄
@copilot Please update your instructions regarding the structure of Given-When-Then tests

Updated .github/copilot-instructions.md with detailed Given-When-Then test structure guidelines, including group structure, shared setup patterns, single-responsibility tests, and a complete example. Commit: 5c3cabf

Copilot finished work on behalf of nielsenko October 8, 2025 07:13
Copilot AI requested a review from nielsenko October 8, 2025 07:13
@nielsenko
Copy link
Collaborator

@copilot Address new review comments

Co-authored-by: nielsenko <22237677+nielsenko@users.noreply.github.com>
Copy link
Contributor Author

Copilot AI commented Oct 8, 2025

@copilot Address new review comments

Addressed the review feedback. The testing guidelines are now more flexible, acknowledging that Given-When-Then structure depends on context. Commit: 3451c5c

Copilot AI requested a review from nielsenko October 8, 2025 07:20
Copilot finished work on behalf of nielsenko October 8, 2025 07:20
@nielsenko nielsenko merged commit 3dbfe36 into main Oct 8, 2025
51 of 52 checks passed
@nielsenko nielsenko deleted the copilot/fix-fc37c190-fcc5-4b1a-ae30-072f410a9339 branch October 8, 2025 07:26
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.

Add NewContext.withRequest(Request req) convenience method

3 participants