Skip to content

fix: clean CI warnings and stabilize tests#78

Open
Sun-sunshine06 wants to merge 1 commit intoOpenCoworkAI:mainfrom
Sun-sunshine06:fix/ci-warning-cleanup
Open

fix: clean CI warnings and stabilize tests#78
Sun-sunshine06 wants to merge 1 commit intoOpenCoworkAI:mainfrom
Sun-sunshine06:fix/ci-warning-cleanup

Conversation

@Sun-sunshine06
Copy link
Copy Markdown
Collaborator

Summary\n- clean the remaining lint warnings in main, renderer, and sandbox modules\n- make credential, path resolver, logger, and plugin runtime tests less brittle across platforms and slower CI runs\n- skip directory symlink-specific assertions when the local Windows environment cannot create symlinks\n\n## Verification\n- npm run lint\n- npm run typecheck\n- npm run test:coverage

Copy link
Copy Markdown
Collaborator

@hqhq1025 hqhq1025 left a comment

Choose a reason for hiding this comment

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

✅ LGTM — Approve

Lint fixes are correct and don't change behavior. Symlink detection logic (supportsDirectorySymlinks() probe) is sound.

Minor observations (non-blocking)

  • plugin-runtime-service.test.ts timeout bump to 15s — the underlying mock-based test shouldn't need this. Worth investigating the root cause separately.
  • ChatView.tsx ResizeObserver useEffect has duplicate eslint-disable-next-line suppression
  • MessageCard.tsx useMemo on contentBlocks busts every render during streaming (rawContent changes each time), providing no real optimization benefit
  • Logger getLogContent polling approach is slightly less reliable than the previous stream finish event. Acceptable for now but noted.

@hqhq1025
Copy link
Copy Markdown
Collaborator

This PR has merge conflicts with the current main branch (likely from the recent skills extraResources change in electron-builder.yml and/or the scrypt fix just merged in #81). Please rebase onto main to resolve.

@hqhq1025
Copy link
Copy Markdown
Collaborator

Hi @Sun-sunshine06, thanks for the comprehensive CI cleanup work!

I've cherry-picked the most valuable parts of this PR into main (commit 5206ba1):

Adopted:

  • lima-bridge.ts / wsl-bridge.ts: .values() iterator simplification
  • dangling-symlink.test.ts: Windows symlink capability detection (supportsDirectorySymlinks())
  • path-resolver-containment.test.ts: MountedPath type import replacing as any
  • store-encryption.test.ts: vi.doUnmock() fix and formatting

Not adopted (with reasons):

  • MessageCard.tsx useMemo — rawContent changes every tick during streaming, so the memo never hits cache (net negative optimization)
  • store/index.ts removeSession — the existing destructuring pattern is the correct Zustand immutable update pattern
  • logger-context.test.ts — polling approach is less reliable than the current stream finish event method
  • useApiConfigState.ts — pure formatting with no functional change, high rebase conflict risk
  • App.tsx useEffect deps — empty deps array is intentional (paired with ref guard for mount-once behavior)
  • ChatView.tsx useCallback — current useRef(fn).current is a zero-overhead stable reference pattern
  • plugin-runtime-service.test.ts type definitions — caused cascading type errors due to PluginCatalogItemV2.catalogSource being a string literal type

Your authorship is preserved via --author on the commit. I'm going to close this PR since the relevant changes are now on main. Thanks again for the contribution! 🙏

hqhq1025 pushed a commit that referenced this pull request Mar 29, 2026
Cherry-pick valuable changes from Sun-sunshine06's PR #78:
- Use .values() iterator in lima-bridge/wsl-bridge to eliminate unused _id
- Add Windows symlink capability detection in dangling-symlink tests
- Replace hardcoded paths with path.join and as-any with MountedPath type
- Use vi.doUnmock instead of vi.unmock in store-encryption tests
@hqhq1025 hqhq1025 closed this Apr 13, 2026
@hqhq1025 hqhq1025 reopened this Apr 13, 2026
@hqhq1025 hqhq1025 closed this Apr 14, 2026
@hqhq1025 hqhq1025 reopened this Apr 14, 2026
@hqhq1025 hqhq1025 added bot-rerun Temporary label for rerunning bot automation and removed bot-rerun Temporary label for rerunning bot automation labels Apr 27, 2026
Copy link
Copy Markdown

@github-actions github-actions Bot left a comment

Choose a reason for hiding this comment

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

Findings

  • No issues identified in the added/modified lines.

Summary

  • Review mode: initial
  • No correctness, security, or maintainability regressions were found in the current diff. Residual testing gap: Windows-specific dangling-symlink behavior is still only covered when directory symlinks are available locally in src/tests/skills/dangling-symlink.test.ts:39, while CI executes tests only on ubuntu-latest in .github/workflows/ci.yml:16.

Testing

  • Not run (automation)

Open Cowork Bot

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.

2 participants