Skip to content

Conversation

@raju-muliyashiya
Copy link
Contributor

@raju-muliyashiya raju-muliyashiya commented Nov 11, 2025

fixes #91400

Replace this paragraph with a description of what this PR is changing or adding, and why. Consider including before/after screenshots.

List which issues are fixed by this PR. You must list at least one issue.

Pre-Review Checklist

  • I read the [Contributor Guide] and followed the process outlined there for submitting PRs.
  • I read the [Tree Hygiene] page, which explains my responsibilities.
  • I read and followed the [relevant style guides] and ran [the auto-formatter].
  • I signed the [CLA].
  • The title of the PR starts with the name of the package surrounded by square brackets, e.g. [shared_preferences]
  • I [linked to at least one issue that this PR fixes] in the description above.
  • I updated pubspec.yaml with an appropriate new version according to the [pub versioning philosophy], or I have commented below to indicate which [version change exemption] this PR falls under[^1].
  • I updated CHANGELOG.md to add a description of the change, [following repository CHANGELOG style], or I have commented below to indicate which [CHANGELOG exemption] this PR falls under[^1].
  • I updated/added any relevant documentation (doc comments with ///).
  • I added updated tests to check the change I am making, or I have commented below to indicate which [test exemption] this PR falls under[^1].
  • All existing and new tests are passing.

@google-cla
Copy link

google-cla bot commented Nov 11, 2025

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

Copy link

@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 "Save As" functionality for the web implementation by moving it from the XFile class into a dedicated helper file. This is a positive change that improves separation of concerns and makes the XFile class cleaner. My review includes suggestions to enhance the new testing mechanism to prevent test pollution and a minor code simplification for better maintainability.

Comment on lines +151 to +153
helpers.anchorElementOverride = (_, __) => mockAnchor;

final XFile file = XFile.fromData(
bytes,
name: textFile.name,
overrides: overrides,
);
final XFile file = XFile.fromData(bytes, name: textFile.name);

Choose a reason for hiding this comment

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

high

Modifying a global variable (helpers.anchorElementOverride) in a test can lead to test pollution, where one test's state affects another. It's crucial to clean up this state after the test runs. You can use addTearDown to ensure the override is reset.

Additionally, the test may create a DOM element that should also be cleaned up to prevent side effects between tests.

        addTearDown(() {
          helpers.anchorElementOverride = null;
          html.document.querySelector('#$crossFileDomElementId')?.remove();
        });
        helpers.anchorElementOverride = (_, __) => mockAnchor;

        final XFile file = XFile.fromData(bytes, name: textFile.name);

Comment on lines +12 to +13
/// Override for creating anchor elements for testing purposes
CreateAnchorElement? anchorElementOverride;

Choose a reason for hiding this comment

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

medium

Using a global variable for testing can lead to test pollution. It's good practice to:

  1. Annotate it with @visibleForTesting to signal its intended use. This requires importing package:meta/meta.dart at the top of the file.
  2. Add a comment reminding developers to clean it up in their tests using tearDown or addTearDown.
/// Override for creating anchor elements for testing purposes.
///
/// This should be reset to `null` in a `tearDown` block to avoid test pollution.
@visibleForTesting
CreateAnchorElement? anchorElementOverride;

Comment on lines +59 to +61
while (target.children.length > 0) {
target.removeChild(target.children.item(0)!);
}

Choose a reason for hiding this comment

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

medium

Instead of a while loop to remove child elements, you can use the more modern and concise Element.replaceChildren() method to clear the container.

  target.replaceChildren();

@raju-muliyashiya raju-muliyashiya deleted the separate-DOM-element-from-XFile-web-class branch November 11, 2025 11:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant