refactor: Improve litegraph preprocessing for i18n collection #5297

snomiao · 2025-09-02T03:34:29Z

This is approach 2 of fixing .github/workflows/i18n.yaml

see also approach 3 (remove declare): #5304

Summary

Replaces script-based litegraph preprocessing with integrated Playwright setup
Removes standalone prebuild/restore scripts in favor of integrated solution
Creates ComfyPageNoUser fixture to avoid user creation conflicts
Ensures pnpm collect-i18n works correctly

Changes

Removed: scripts/prebuild-litegraph.js and scripts/restore-litegraph.js
Added: browser_tests/i18nSetup.ts for integrated preprocessing
Added: browser_tests/fixtures/ComfyPageNoUser.ts to avoid user conflicts
Updated: playwright.i18n.config.ts to use new setup/teardown
Simplified: collect-i18n command no longer needs manual prebuild/restore

Test Plan

Verify pnpm collect-i18n command works correctly
Confirm litegraph files are preprocessed and restored properly
Ensure i18n files are generated successfully

🤖 Generated with Claude Code

┆Issue is synchronized with this Notion page by Unito

…elds in litegraph - Add prebuild script that temporarily removes 'declare' keyword from litegraph TypeScript files - Add restore script to revert files to original state after i18n collection - Update collect-i18n script to use prebuild/restore workflow - Add babel dependencies for TypeScript transformation - Update playwright.i18n.config.ts with global setup/teardown This fix addresses an issue where Playwright's Babel transformation couldn't handle TypeScript 'declare' fields in litegraph classes, causing the i18n collection to fail. Fixes the issue where 'pnpm collect-i18n' would fail with: "TypeScript 'declare' fields must first be transformed by @babel/plugin-transform-typescript"

…r locale updates

… adjusting spacing and line breaks

…d Playwright setup - Remove prebuild-litegraph.js and restore-litegraph.js scripts - Add i18nSetup.ts module for litegraph TypeScript 'declare' keyword preprocessing - Create ComfyPageNoUser fixture to avoid user creation conflicts in i18n tests - Update playwright.i18n.config.ts to use integrated setup/teardown - Simplify collect-i18n command to just run Playwright tests - Ensure pnpm collect-i18n works correctly 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>

github-actions · 2025-09-02T03:34:40Z

🎭 Playwright Test Results

✅ Tests completed successfully!

⏰ Completed at: 09/12/2025, 03:39:45 AM UTC

📊 Test Reports by Browser

✅ chromium: View Report
✅ chromium-2x: View Report
✅ chromium-0.5x: View Report
✅ mobile-chrome: View Report

🎉 Click on the links above to view detailed test results for each browser configuration.

socket-security · 2025-09-02T03:35:03Z

Review the following changes in direct dependencies. Learn more about Socket for GitHub.

Diff	Package	Supply Chain Security	Vulnerability	Quality	Maintenance	License

View full report

The NoUser fixture doesn't work properly in CI. Tests will handle user creation properly with the standard fixture.

Instead of hardcoding the list of files, dynamically find all TypeScript files in litegraph that contain 'declare' keywords using glob pattern matching.

- Replace synchronous fs operations with async fs.promises - Use Promise.all for parallel file processing - Improves performance when processing multiple files

snomiao · 2025-09-02T16:44:20Z

the playwright deploy link is here

Playwright Test Report

@christian-byrne

"Can load audio" test failed by a small diff on node name

Maybe someone forgot to update playwright expectation or we set threshold too small?

christian-byrne · 2025-09-02T20:54:07Z

The display name change from "LoadAudio" -> "Load Audio" was added 4 months ago comfyanonymous/ComfyUI@b4abca8. I wonder why the test only started failing now?

snomiao · 2025-09-02T21:14:58Z

its Ready for review, current CI Fails:

lint-and-format -- fixed by PR - [bugfix] Fix lint-and-format workflow detached HEAD issue by snomiao · Pull Request #5305 · Comfy-Org/ComfyUI_frontend
can-load-audio -- will be fixed by regenerate playwright expectation

@christian-byrne

- Fix trailing whitespace - Add missing newlines at end of files

… FullConfig parameter for better integration with Playwright's testing framework

…on to ensure they are included in the build process

…etter clarity and maintainability feat(knip.config.ts): add support for vite and vitest configurations to enhance project structure and testing capabilities

…CSS integration in the project

snomiao · 2025-09-12T01:03:31Z

got tailwindcss unsed error

> knip --cache

Unused devDependencies (2)
tailwindcss          package.json:83:6
tailwindcss-primeui  package.json:84:6
 ELIFECYCLE  Command failed with exit code 1.
husky - pre-push script failed (code 1)
error: failed to push some refs to 'github.com:Comfy-Org/ComfyUI_frontend.git'

…plugins list to support Tailwind CSS integration in the project

christian-byrne · 2025-09-12T01:15:32Z

got tailwindcss unsed error

Is it fixed if you rebase?

browser_tests/i18nSetup.ts

browser_tests/globalTeardownWithI18n.ts

claude

Comprehensive PR Review

This review is generated by Claude. It may not always be accurate, as with human reviewers. If you believe that any of the comments are invalid or incorrect, please state why for each. For others, please implement the changes in one way or another.

Review Summary

PR: refactor: Improve litegraph preprocessing for i18n collection (#5297)
Impact: 423 additions, 296 deletions across 7 files

Issue Distribution

Critical: 0
High: 0
Medium: 3
Low: 3

Category Breakdown

Architecture: 0 issues
Security: 0 issues
Performance: 1 issues
Code Quality: 5 issues

Key Findings

Architecture & Design

The PR introduces a well-structured approach to litegraph preprocessing by consolidating functionality into dedicated modules. The integration with Playwright's global setup/teardown lifecycle is architecturally sound. The approach of replacing standalone scripts with integrated Playwright setup follows good practices for test infrastructure.

Security Considerations

No security vulnerabilities identified. The file preprocessing operations are contained within the test environment and don't expose any security risks.

Performance Impact

The solution includes a performance consideration noted in comments (70ms for 90+ file reads). While functional, there's room for optimization in the file scanning approach through batching or streaming operations.

Integration Points

The changes properly integrate with the existing Playwright configuration and don't break existing test infrastructure. The knip.config.ts updates ensure build tools are properly recognized.

Positive Observations

Clean modular design with proper separation of concerns
Good error handling with informative messages
Proper use of TypeScript types and modern async/await patterns
Integration with existing toolchain (Playwright, knip)
Memory-based backup strategy avoids filesystem pollution

References

Next Steps

Address medium priority issues for better maintainability
Consider performance optimizations for file scanning
Ensure consistent TypeScript import patterns
Review global state management approach

This is a comprehensive automated review. For architectural decisions requiring human judgment, please request additional manual review.

snomiao · 2025-09-12T01:21:48Z

got tailwindcss unsed error

Is it fixed if you rebase?

I've got it because rebased

Refactored preprocessLitegraph() to read each file only once instead of twice. Previously files were read once to check for 'declare' keywords and again to process them. Now files are read once and processed inline if they contain 'declare' keywords, improving performance.

…cessing failure

… import for FullConfig to improve clarity and maintainability

christian-byrne

LGTM!

snomiao · 2025-09-12T02:21:57Z

LGTM!

In the final-test PR - Test: Update locales workflow for sno-fix-playwright-babel-test branch by snomiao · Pull Request #5503 · Comfy-Org/ComfyUI_frontend, I've got a new error never seen before, working on it now @christian-byrne

> @comfyorg/[email protected] collect-i18n /home/runner/work/ComfyUI_frontend/ComfyUI_frontend/ComfyUI_frontend
> npx playwright test --config=playwright.i18n.config.ts

GitCommitInfo: timeout of 3000ms exceeded while running "git fetch origin dfcbbec2b95ba4392365ce9418618e1017b82240"
Preprocessing litegraph files for i18n collection...
  ✓ Processed LGraphNode.ts
  ✓ Processed subgraph/SubgraphOutput.ts
  ✓ Processed widgets/BaseWidget.ts
  ✓ Processed subgraph/SubgraphNode.ts
  ✓ Processed subgraph/SubgraphInput.ts
  ✓ Processed subgraph/EmptySubgraphInput.ts
  ✓ Processed subgraph/EmptySubgraphOutput.ts
  Processed 7 files with declare keywords
TypeError: Module "file:///home/runner/work/ComfyUI_frontend/ComfyUI_frontend/ComfyUI_frontend/src/locales/ar/commands.json" needs an import attribute of "type: json"
Restoring original litegraph files...
  ✓ Restored LGraphNode.ts
  ✓ Restored widgets/BaseWidget.ts
  ✓ Restored subgraph/SubgraphOutput.ts
  ✓ Restored subgraph/SubgraphNode.ts
  ✓ Restored subgraph/SubgraphInput.ts
  ✓ Restored subgraph/EmptySubgraphOutput.ts
  ✓ Restored subgraph/EmptySubgraphInput.ts**

pending - fix: Add JSON import assertions for Node.js ESM compatibility by snomiao · Pull Request #5507 · Comfy-Org/ComfyUI_frontend

…babel branch for temporary Playwright issue fix

snomiao · 2025-09-26T06:04:23Z

close this as not necessary anymore because we solved locale workflow problem here: - fix(collect-i18n-node-defs): refactor to run ComfyNodeDefImpl only in browser context by snomiao · Pull Request #5775 · Comfy-Org/ComfyUI_frontend

snomiao and others added 5 commits September 1, 2025 23:21

fix(i18n.yaml): update job condition to include 'sno-fix' branches fo…

c183026

…r locale updates

chore(i18n.yaml): format YAML file for consistency and readability by…

61f9628

… adjusting spacing and line breaks

Update locales [skip ci]

83de398

snomiao and others added 5 commits September 2, 2025 13:54

Merge branch 'main' into sno-fix-playwright-babel

29add8f

fix: Remove ComfyPageNoUser fixture and revert to standard fixture

ea917ba

The NoUser fixture doesn't work properly in CI. Tests will handle user creation properly with the standard fixture.

Update locales [skip ci]

4e56bae

refactor: Use glob pattern to find litegraph files with declare keywords

2403b47

Instead of hardcoding the list of files, dynamically find all TypeScript files in litegraph that contain 'declare' keywords using glob pattern matching.

refactor: Use async fs/promises for better performance

0a3f878

- Replace synchronous fs operations with async fs.promises - Use Promise.all for parallel file processing - Improves performance when processing multiple files

Merge branch 'main' into sno-fix-playwright-babel

3700457

snomiao mentioned this pull request Sep 2, 2025

[bugfix] Fix lint-and-format workflow detached HEAD issue #5305

Merged

3 tasks

snomiao marked this pull request as ready for review September 2, 2025 21:13

snomiao requested review from Yorha4D, KarryCharon, shinshin86 and a team as code owners September 2, 2025 21:13

dosubot bot added the size:XL This PR changes 500-999 lines, ignoring generated files. label Sep 2, 2025

snomiao and others added 3 commits September 2, 2025 21:17

style: Apply Prettier formatting

f037567

- Fix trailing whitespace - Add missing newlines at end of files

Update locales [skip ci]

2a611cb

Merge branch 'main' into sno-fix-playwright-babel

adbfd59

dosubot bot added size:L This PR changes 100-499 lines, ignoring generated files. and removed size:XL This PR changes 500-999 lines, ignoring generated files. labels Sep 3, 2025

Update locales [skip ci]

32b8a1e

christian-byrne added area:i18n Anything related to translation area:CI/CD labels Sep 12, 2025

snomiao added 4 commits September 12, 2025 00:49

refactor(tests): update global setup and teardown functions to accept…

ad111f0

… FullConfig parameter for better integration with Playwright's testing framework

chore(knip.config.ts): add i18n related test files to the configurati…

98318c8

…on to ensure they are included in the build process

refactor(knip.config.ts): simplify and reorganize configuration for b…

a79d87d

…etter clarity and maintainability feat(knip.config.ts): add support for vite and vitest configurations to enhance project structure and testing capabilities

feat(knip.config.ts): add tailwind configuration to support Tailwind …

4984e96

…CSS integration in the project

feat(knip.config.ts): add tailwindcss and tailwindcss-primeui to the …

08d4c07

…plugins list to support Tailwind CSS integration in the project

christian-byrne added the claude-review Add to trigger a PR code review from Claude Code label Sep 12, 2025