Skip to content

Conversation

@kevmoo
Copy link
Contributor

@kevmoo kevmoo commented Nov 17, 2025

Move getFinalTargetUri to ProxyRule class
Fix doc comments
Pares URI immediately when creating RegexProxyRule

Move getFinalTargetUri to ProxyRule class
Fix doc comments
Pares URI immediately when creating RegexProxyRule
@kevmoo kevmoo requested a review from bkonyi November 17, 2025 21:56
@github-actions github-actions bot added the tool Affects the "flutter" command-line tool. See also t: labels. label Nov 17, 2025
@kevmoo kevmoo requested a review from mdebbar November 17, 2025 21:56
final Uri finalTargetUrl = rule.finalTargetUri(request.requestedUri);
try {
final shelf.Request proxyBackendRequest = proxyRequest(request, finalTargetUrl);
final shelf.Response proxyResponse = await proxyHandler(targetUri)(proxyBackendRequest);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't like createFunction(args)(invokeArgs). Fixed.

Copy link
Contributor

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

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

Code Review

This pull request refactors the proxy logic by moving getFinalTargetUri into the ProxyRule class and parsing the target URI earlier in the RegexProxyRule constructor. This is a good cleanup that improves encapsulation and efficiency. The changes are well-executed, but I've found a couple of issues with error handling that were introduced by this refactoring. Specifically, invalid target URIs in the configuration will now cause an unhandled exception. I've also added a minor comment about missing documentation to adhere to the style guide.

Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
RegexProxyRule({required RegExp pattern, required String target, String? replacement})
: _pattern = pattern,
_target = target,
: targetUri = Uri.parse(target),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

If the provided target is invalid, better to find out HERE than when the proxy is handling requests!

@kevmoo kevmoo added the autosubmit Merge PR when tree becomes green via auto submit App label Nov 18, 2025
@auto-submit auto-submit bot added this pull request to the merge queue Nov 18, 2025
Merged via the queue into master with commit b1f4694 Nov 18, 2025
145 of 146 checks passed
@auto-submit auto-submit bot deleted the proxy_fix_follow_up branch November 18, 2025 21:01
@flutter-dashboard flutter-dashboard bot removed the autosubmit Merge PR when tree becomes green via auto submit App label Nov 18, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Nov 19, 2025
auto-submit bot pushed a commit to flutter/packages that referenced this pull request Nov 19, 2025
flutter/flutter@cb7b7df...de4be4f

2025-11-19 [email protected] Roll Skia from 2054d87c6a85 to b5dc8c3494ac (1 revision) (flutter/flutter#178793)
2025-11-19 [email protected] Roll Skia from a30b02d57637 to 2054d87c6a85 (2 revisions) (flutter/flutter#178787)
2025-11-19 [email protected] Roll Skia from 547e3e5441b4 to a30b02d57637 (3 revisions) (flutter/flutter#178782)
2025-11-19 [email protected] Fix train hopping animation status listeners (flutter/flutter#178372)
2025-11-19 [email protected] Make sure that a ReorderableListView doesn't crash in 0x0 environment (flutter/flutter#177646)
2025-11-19 [email protected] Roll Skia from 9ce01a452f63 to 547e3e5441b4 (1 revision) (flutter/flutter#178775)
2025-11-19 [email protected] Roll Dart SDK from 1ed6b56bb323 to f7e9bd245fd9 (1 revision) (flutter/flutter#178774)
2025-11-19 [email protected] Roll Skia from f3ddc700abc7 to 9ce01a452f63 (8 revisions) (flutter/flutter#178769)
2025-11-19 [email protected] Make sure that a TabPageSelector doesn't crash in 0x0 environment (flutter/flutter#178156)
2025-11-19 [email protected] Small cleanup in `DeferredComponentManager.java‎` (flutter/flutter#178585)
2025-11-19 [email protected] Roll Dart SDK from a33149cb6643 to 1ed6b56bb323 (1 revision) (flutter/flutter#178763)
2025-11-18 [email protected] Roll Skia from 8557300f84c2 to f3ddc700abc7 (5 revisions) (flutter/flutter#178751)
2025-11-18 [email protected] Remove unnecessary `String.valueOf` in `TextInputChannel.java` (flutter/flutter#178592)
2025-11-18 [email protected] [tool] Further cleanup of proxy logic (flutter/flutter#178683)
2025-11-18 [email protected] Restore OpenGL state modified by fl_compositor_opengl_present_layers (flutter/flutter#178697)
2025-11-18 [email protected] Replace `equals("")` with `isEmpty` in `SpellCheckPlugin.java‎` (flutter/flutter#178596)
2025-11-18 [email protected] Roll Dart SDK from 312845b06afc to a33149cb6643 (2 revisions) (flutter/flutter#178738)
2025-11-18 [email protected] Roll Skia from ca906091e199 to 8557300f84c2 (2 revisions) (flutter/flutter#178739)
2025-11-18 98614782+auto-submit[bot]@users.noreply.github.com Reverts "Add framework-side hitTestBehavior support for Semantics widget and apply to ModalRoute (#177570)" (flutter/flutter#178744)
2025-11-18 [email protected] Roll Packages from ce44ebb to 34746bb (6 revisions) (flutter/flutter#178734)

If this roll has caused a breakage, revert this CL and stop the roller
using the controls here:
https://autoroll.skia.org/r/flutter-packages
Please CC [email protected],[email protected] on the revert to ensure that a human
is aware of the problem.

To file a bug in Packages: https://github.com/flutter/flutter/issues/new/choose

To report a problem with the AutoRoller itself, please file a bug:
https://issues.skia.org/issues/new?component=1389291&template=1850622

Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
IvoneDjaja pushed a commit to IvoneDjaja/flutter that referenced this pull request Nov 22, 2025
Move getFinalTargetUri to ProxyRule class
Fix doc comments
Pares URI immediately when creating RegexProxyRule

---------

Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
mboetger pushed a commit to mboetger/flutter that referenced this pull request Dec 2, 2025
Move getFinalTargetUri to ProxyRule class
Fix doc comments
Pares URI immediately when creating RegexProxyRule

---------

Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
reidbaker pushed a commit to AbdeMohlbi/flutter that referenced this pull request Dec 10, 2025
Move getFinalTargetUri to ProxyRule class
Fix doc comments
Pares URI immediately when creating RegexProxyRule

---------

Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

tool Affects the "flutter" command-line tool. See also t: labels.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants