Skip to content

Conversation

@rogaldh
Copy link
Contributor

@rogaldh rogaldh commented Apr 25, 2025

Description

PR runs Playwright tests at CI

Type of change

  • New feature

Testing

pnpm test:ci

Checklist

  • My code follows the project's style guidelines
  • I have added tests that prove my fix/feature works
  • All tests pass locally and in CI
  • CI/CD checks pass

Important

Adds Playwright tests to CI, improves error handling in test setup, and updates dependencies in package.json.

  • CI Integration:
    • Adds Playwright tests to CI by modifying vitest.workspace.ts to enable browser testing with Playwright.
  • Error Handling:
    • Wraps beforeAll in a try-catch block in .storybook/vitest.setup.ts to log errors during setup.
  • Dependencies:
    • Updates package.json to include @storybook/addon-vitest and modifies test:sb script.
    • Removes @chromatic-com/storybook and updates @storybook/experimental-nextjs-vite version.

This description was created by Ellipsis for 5891871. You can customize this summary. It will automatically update as commits are pushed.

@vercel
Copy link

vercel bot commented Apr 25, 2025

@rogaldh is attempting to deploy a commit to the Solana Foundation Team on Vercel.

A member of the Team first needs to authorize it.

Copy link
Contributor

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

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

Caution

Changes requested ❌

Reviewed everything up to 28b1511 in 2 minutes and 57 seconds. Click for details.
  • Reviewed 2124 lines of code in 35 files
  • Skipped 1 files when reviewing.
  • Skipped posting 15 draft comments. View those below.
  • Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. .github/workflows/ci.yaml:55
  • Draft comment:
    Ensure that installing Playwright dependencies via pnpx is intentional; consider using 'pnpm exec' if consistent.
  • Reason this comment was not posted:
    Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 20% vs. threshold = 50% Both pnpx and pnpm exec are valid ways to run binaries. While consistency is nice, this is more of a style preference than a functional issue. The current command will work correctly. The comment is asking for confirmation of intention rather than pointing out a real problem. The suggestion could improve consistency in the codebase, and inconsistent package manager usage might confuse future maintainers. While consistency is good, this is a minor stylistic preference that doesn't affect functionality. The comment starts with "Ensure that..." which violates our rules about not asking for confirmation. Delete the comment as it's asking for confirmation of intention and suggesting a style change that isn't functionally necessary.
2. .storybook/main.ts:31
  • Draft comment:
    Remove or document the commented-out storybookTest plugin configuration if no longer needed.
  • Reason this comment was not posted:
    Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 20% vs. threshold = 50% Commented-out code can be a code smell and cleaning it up is generally good practice. However, this appears to be part of a larger work-in-progress refactoring of the _viteFinal configuration, with multiple commented sections. The author likely left these comments intentionally while working through different approaches. The comment doesn't add much value in this context. The commented code could be technical debt that needs addressing. The unused import also suggests this needs cleanup. While technical debt is important, this appears to be active development work rather than abandoned code, given the multiple variations being tried. The author is likely aware of the comments they just added. The comment should be deleted as it's pointing out something the author is actively working with and is likely temporary during development.
3. app/address/[address]/layout.tsx:217
  • Draft comment:
    Header component now wraps AccountHeader; verify that behaviours from removed MetaplexNFTHeader are preserved.
  • Reason this comment was not posted:
    Comment did not seem useful. Confidence is useful = 0% <= threshold 50% The comment is asking the PR author to verify that behaviors are preserved, which is against the rules. It does not provide a specific suggestion or ask for a specific test to be written. Therefore, it should be removed.
4. tailwind.config.ts:30
  • Draft comment:
    Good use of getScreenDim to adjust breakpoints—but ensure the 1px shift aligns with design requirements.
  • Reason this comment was not posted:
    Confidence changes required: 20% <= threshold 50% None
5. vite.config.mts:21
  • Draft comment:
    Alias configuration looks thorough; consider consolidating duplicate aliases for clarity.
  • Reason this comment was not posted:
    Confidence changes required: 30% <= threshold 50% None
6. vitest.workspace.ts:18
  • Draft comment:
    The workspace uses a Storybook test plugin with nodePolyfills; double-check that the globals configuration meets all test cases.
  • Reason this comment was not posted:
    Confidence changes required: 50% <= threshold 50% None
7. app/components/account/TokenAccountSection.tsx:212
  • Draft comment:
    The 'Bridged Asset Contract' row uses tokenInfo.extensions.bridgeContract in the href, but it should likely reference tokenInfo.extensions.assetContract instead. Please verify and fix the URL.
  • Reason this comment was not posted:
    Comment was on unchanged code.
8. .storybook/main.ts:31
  • Draft comment:
    There is commented-out code for the storybookTest plugin. If no longer needed, consider removing it to clean up the configuration.
  • Reason this comment was not posted:
    Confidence changes required: 50% <= threshold 50% None
9. app/tx/[signature]/page-client.tsx:107
  • Draft comment:
    The auto-refresh effect uses the status dependency to update zeroConfirmationRetries. Consider reviewing the dependency array of the useEffect hook to ensure stable behavior if status toggles frequently.
  • Reason this comment was not posted:
    Confidence changes required: 50% <= threshold 50% None
10. vite.config.mts:21
  • Draft comment:
    Aliases for '@/app', '@app', etc. are repeated. Consider consolidating them to avoid duplication.
  • Reason this comment was not posted:
    Comment was not on a location in the diff, so it can't be submitted as a review comment.
11. test-setup.ts:13
  • Draft comment:
    Overriding Uint8Array[Symbol.hasInstance] with Buffer.isBuffer is a known hack; ensure it’s well documented and reviewed for potential edge cases.
  • Reason this comment was not posted:
    Comment was not on a location in the diff, so it can't be submitted as a review comment.
12. .github/workflows/ci.yaml:10
  • Draft comment:
    Typo detected in the inline comment on line 10. It currently reads 'cash folder for pnpm_store, nextjs and playwright later on'. Please update 'cash' to 'cache' for clarity.
  • Reason this comment was not posted:
    Comment was not on a location in the diff, so it can't be submitted as a review comment.
13. app/components/account/AccountHeader.tsx:168
  • Draft comment:
    Typo: The branding for 'Coingecko' should be properly capitalized as 'CoinGecko' to match the official styling.
  • Reason this comment was not posted:
    Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 0% vs. threshold = 50% While technically correct about the official branding, this is a very minor UI detail. Our rules specifically say not to comment on pure frontend UI changes and to assume the author made UI choices correctly. This feels more like a style preference than a critical issue requiring a code change. The branding consistency could be important for professionalism and legal compliance. Incorrect capitalization could potentially confuse users about whether they're dealing with the official service. While brand consistency has merit, this is still fundamentally a UI text change. The link still works correctly and users can clearly understand what service is being referenced. Our rules explicitly state to avoid pure UI feedback. This comment should be removed as it deals with UI text styling, which our rules explicitly say to ignore.
14. app/components/account/TokenAccountSection.tsx:212
  • Draft comment:
    Typo: In the asset contract row, the anchor tag's href incorrectly references tokenInfo.extensions.bridgeContract instead of tokenInfo.extensions.assetContract. Please update the href to use tokenInfo.extensions.assetContract so that it links to the correct URL.
  • Reason this comment was not posted:
    Comment was on unchanged code.
15. app/components/account/token-extensions/TokenExtensionsStatusRow.tsx:12
  • Draft comment:
    Typo detected: The word 'compoent' should be corrected to 'component' in the comment on line 12.
  • Reason this comment was not posted:
    Comment was not on a location in the diff, so it can't be submitted as a review comment.

Workflow ID: wflow_cFTgJ18xqSXPDEnj

You can customize Ellipsis by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.

@rogaldh rogaldh marked this pull request as draft April 25, 2025 15:54
@rogaldh
Copy link
Contributor Author

rogaldh commented Apr 25, 2025

Facing issue at the CI side:

Error: Vitest failed to find the current suite. This is a bug in Vitest. Please, open an issue with reproduction.
| [0]  ❯ node_modules/.pnpm/@[email protected]_@[email protected][email protected][email protected]_4lkvhawbglztbrxtzxb7jz3muu/node_modules/@storybook/experimental-addon-test/dist/vitest-plugin/setup-file.mjs:8:29
| [0]       6| \x1B[34mClick to debug the error directly in Storybook: ${storyUrl}\x1...
| [0]       7|
| [0]       8| ${currentError.message}`;}};afterEach(modifyErrorMessage);
| [0]        |                             ^
| [0]       9|
| [0]      10| export { modifyErrorMessage };

@rogaldh
Copy link
Contributor Author

rogaldh commented May 14, 2025

I removed the script for playwright from CI, but we can run it locally. Executing it at CI will be the next step. It seems reasonable to implement this in a separate PR.

@rogaldh rogaldh marked this pull request as ready for review May 14, 2025 18:01
Copy link
Contributor

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

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

Caution

Changes requested ❌

Reviewed everything up to 5891871 in 1 minute and 42 seconds. Click for details.
  • Reviewed 70 lines of code in 3 files
  • Skipped 1 files when reviewing.
  • Skipped posting 4 draft comments. View those below.
  • Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. package.json:17
  • Draft comment:
    Changed 'test:sb' script to non-watch mode; consider adding an alternate script for watch mode if interactive debugging is needed.
  • Reason this comment was not posted:
    Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 20% vs. threshold = 50% The change deliberately removes watch mode from the storybook tests. This appears intentional since there's already a pattern of having separate watch/non-watch scripts (test vs test:ci). The comment suggests adding complexity that may not be needed - if watch mode was needed, the author would likely have kept it. The comment is more of a suggestion than pointing out a clear issue. The author may have removed watch mode for a specific reason, like improving CI performance. Having both watch and non-watch modes could be useful for different development workflows. While having both modes could be nice, this is more of an optional enhancement than a clear issue. The default should be to trust the author's judgment on their testing workflow needs. Delete the comment. It's making a suggestion about a deliberate change without strong evidence that an additional watch mode script is actually needed.
2. package.json:49
  • Draft comment:
    The pinned experimental version for @storybook/addon-vitest should be documented to clarify its necessity and potential caveats.
  • Reason this comment was not posted:
    Comment did not seem useful. Confidence is useful = 0% <= threshold 50% The comment is asking for documentation on a specific version of a dependency, which is not allowed according to the rules. It doesn't provide a specific code suggestion or ask for a test to be written.
3. package.json:103
  • Draft comment:
    Using '^8.6.12' for @storybook/experimental-nextjs-vite allows patch updates; verify this change is intentional and compatible with your setup.
  • Reason this comment was not posted:
    Comment did not seem useful. Confidence is useful = 0% <= threshold 50% This comment is related to a dependency version change. It asks the author to verify the change, which is against the rules. The comment does not provide a specific suggestion or highlight a specific issue with the code.
4. vitest.workspace.ts:32
  • Draft comment:
    Switching browser configuration from a single 'name' to an 'instances' array improves configurability; ensure this new syntax is fully supported by the Playwright provider.
  • Reason this comment was not posted:
    Comment did not seem useful. Confidence is useful = 0% <= threshold 50% The comment is asking the author to ensure that the new syntax is fully supported by the Playwright provider. This falls under the category of asking the author to ensure something is tested or verified, which is against the rules.

Workflow ID: wflow_EDakPFPkrsxuxrKb

You can customize Ellipsis by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.

@ngundotra
Copy link
Contributor

Can you squash the git history?

chore: install playwright at CI and run pw-suite
@rogaldh rogaldh force-pushed the chore/add-playwright-test-to-ci branch from c8ee98c to 7f0abbf Compare May 15, 2025 20:24
@rogaldh
Copy link
Contributor Author

rogaldh commented May 15, 2025

Can you squash the git history?

@ngundotra done

<td className="text-lg-end">
<Copyable text={assetContractAddress}>
<a href={tokenInfo.extensions.bridgeContract} target="_blank" rel="noreferrer">
<a href={tokenInfo.extensions.assetContract} target="_blank" rel="noreferrer">
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this getting changed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@rogaldh rogaldh marked this pull request as draft September 15, 2025 15:20
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.

3 participants