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
Contributor

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
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