TS6 compatibility fixes + @teambit/harmony 0.4.12#10466
Open
davidfirst wants to merge 5 commits into
Open
Conversation
Code Review by Qodo
1. Hardcoded ANSI + ✓/ output
|
PR Summary by QodoTS6 compatibility fixes and bump @teambit/harmony to 0.4.12
AI Description
Diagram
High-Level Assessment
Files changed (8)
|
Code Review by QodoNew Review StartedThis review has been superseded by a new analysis |
![qodo-free-for-open-source-projects[bot]](https://avatars.githubusercontent.com/in/1068000?s=60&v=4){kind=small}
Comment on lines
+18
to
+46
| RED='\033[0;31m' | ||
| GREEN='\033[0;32m' | ||
| YELLOW='\033[1;33m' | ||
| NC='\033[0m' # No Color | ||
|
|
||
| FIX=false | ||
| if [ "${1:-}" = "--fix" ]; then | ||
| FIX=true | ||
| fi | ||
|
|
||
| WORKSPACE_FILE="workspace.jsonc" | ||
| CONFIG_FILE=".circleci/config.yml" | ||
|
|
||
| echo "Checking @teambit/harmony version synchronization..." | ||
|
|
||
| # Source of truth: the harmony pins in workspace.jsonc. There are two (the | ||
| # dependency policy and the dependency-resolver overrides); they must agree. | ||
| WS_VERSIONS=$(grep -oE '"@teambit/harmony": *"[0-9]+\.[0-9]+\.[0-9]+"' "$WORKSPACE_FILE" | grep -oE '[0-9]+\.[0-9]+\.[0-9]+' | sort -u) | ||
| WS_COUNT=$(echo "$WS_VERSIONS" | grep -c .) | ||
|
|
||
| if [ -z "$WS_VERSIONS" ]; then | ||
| echo -e "${RED}✗ ERROR: no @teambit/harmony pin found in ${WORKSPACE_FILE}${NC}" | ||
| exit 1 | ||
| fi | ||
|
|
||
| if [ "$WS_COUNT" -ne 1 ]; then | ||
| echo -e "${RED}✗ ERROR: ${WORKSPACE_FILE} has inconsistent @teambit/harmony pins:${NC}" | ||
| echo "$WS_VERSIONS" | sed 's/^/ /' | ||
| echo -e "${YELLOW}Fix workspace.jsonc first so all @teambit/harmony pins match, then re-run.${NC}" |
There was a problem hiding this comment.
1. Hardcoded ansi + ✓/✗ output 📘 Rule violation ⚙ Maintainability
scripts/check-harmony-version-sync.sh introduces hardcoded ANSI color escape sequences and Unicode symbols (✓/✗) in CLI output. This violates the requirement to avoid ad-hoc styling/symbols and instead use the shared CLI output formatting toolkit for consistent output.
Agent Prompt
## Issue description
The new bash script prints hardcoded ANSI color codes and Unicode symbols (`✓`/`✗`) in its output, which is disallowed by the CLI output compliance rule.
## Issue Context
This repository requires CLI output to avoid ad-hoc styling/symbols and instead use the shared formatting toolkit from `@teambit/cli` (or, for non-TS scripts, fall back to plain, unstyled ASCII output).
## Fix Focus Areas
- scripts/check-harmony-version-sync.sh[18-46]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+33
to
+56
| # Source of truth: the harmony pins in workspace.jsonc. There are two (the | ||
| # dependency policy and the dependency-resolver overrides); they must agree. | ||
| WS_VERSIONS=$(grep -oE '"@teambit/harmony": *"[0-9]+\.[0-9]+\.[0-9]+"' "$WORKSPACE_FILE" | grep -oE '[0-9]+\.[0-9]+\.[0-9]+' | sort -u) | ||
| WS_COUNT=$(echo "$WS_VERSIONS" | grep -c .) | ||
|
|
||
| if [ -z "$WS_VERSIONS" ]; then | ||
| echo -e "${RED}✗ ERROR: no @teambit/harmony pin found in ${WORKSPACE_FILE}${NC}" | ||
| exit 1 | ||
| fi | ||
|
|
||
| if [ "$WS_COUNT" -ne 1 ]; then | ||
| echo -e "${RED}✗ ERROR: ${WORKSPACE_FILE} has inconsistent @teambit/harmony pins:${NC}" | ||
| echo "$WS_VERSIONS" | sed 's/^/ /' | ||
| echo -e "${YELLOW}Fix workspace.jsonc first so all @teambit/harmony pins match, then re-run.${NC}" | ||
| exit 1 | ||
| fi | ||
|
|
||
| EXPECTED="$WS_VERSIONS" | ||
| echo "Found @teambit/harmony in ${WORKSPACE_FILE}: $EXPECTED" | ||
|
|
||
| # The config.yml overrides look like: | ||
| # pnpm.overrides.@teambit/harmony" --values "0.4.12" | ||
| CONFIG_VERSIONS=$(grep -oE 'pnpm\.overrides\.@teambit/harmony" --values "[0-9]+\.[0-9]+\.[0-9]+"' "$CONFIG_FILE" | grep -oE '[0-9]+\.[0-9]+\.[0-9]+' | sort -u) | ||
|
|
There was a problem hiding this comment.
2. Strict harmony version regex 🐞 Bug ⚙ Maintainability
scripts/check-harmony-version-sync.sh only recognizes @teambit/harmony pins that match an exact numeric "x.y.z" regex, so pins like "^0.4.12" or "0.4.13-rc.1" will be treated as missing/mismatched and fail the new CI step. This can unexpectedly block future bumps if the pinning format changes (ranges/prereleases).
Agent Prompt
### Issue description
`check-harmony-version-sync.sh` extracts versions using regexes that only match exact `x.y.z` strings. If the repo ever uses semver ranges (e.g. `^0.4.12`) or prereleases/build metadata (e.g. `0.4.13-rc.1`), the script will report “no pin found” / mismatch and fail CI.
### Issue Context
The script is now executed in CircleCI (`check_env_cache_sync` job). It is intended to prevent drift between `workspace.jsonc` and the `.circleci/config.yml` `pnpm.overrides.@teambit/harmony` overrides.
### Fix Focus Areas
- scripts/check-harmony-version-sync.sh[35-56]
### Suggested fix
Choose one:
1) **If exact pins are required by policy**: update script comments/error messages to explicitly state it enforces exact `x.y.z` pins (and consider validating that the pins are exact to make failures clearer).
2) **If flexibility is desired**: broaden the regex to accept semver prerelease/build metadata and optional `^`/`~`, e.g.:
- `\^?([0-9]+\.[0-9]+\.[0-9]+(?:-[0-9A-Za-z.-]+)?(?:\+[0-9A-Za-z.-]+)?)`
and use that capture for both `workspace.jsonc` and `.circleci/config.yml` extraction.
3) **Most robust**: replace grep-regex parsing with a small `node` snippet that parses JSONC/YAML and reads the relevant fields explicitly.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
|
Code review by qodo was updated up to the latest commit 03ef52b |
GiladShoham
approved these changes
Jul 2, 2026
# Conflicts: # pnpm-lock.yaml
Comment on lines
+16
to
+41
| set -e | ||
|
|
||
| RED='\033[0;31m' | ||
| GREEN='\033[0;32m' | ||
| YELLOW='\033[1;33m' | ||
| NC='\033[0m' # No Color | ||
|
|
||
| FIX=false | ||
| if [ "${1:-}" = "--fix" ]; then | ||
| FIX=true | ||
| fi | ||
|
|
||
| WORKSPACE_FILE="workspace.jsonc" | ||
| CONFIG_FILE=".circleci/config.yml" | ||
|
|
||
| echo "Checking @teambit/harmony version synchronization..." | ||
|
|
||
| # Source of truth: the harmony pins in workspace.jsonc. There are two (the | ||
| # dependency policy and the dependency-resolver overrides); they must agree. | ||
| WS_VERSIONS=$(grep -oE '"@teambit/harmony": *"[0-9]+\.[0-9]+\.[0-9]+"' "$WORKSPACE_FILE" | grep -oE '[0-9]+\.[0-9]+\.[0-9]+' | sort -u) | ||
| WS_COUNT=$(echo "$WS_VERSIONS" | grep -c .) | ||
|
|
||
| if [ -z "$WS_VERSIONS" ]; then | ||
| echo -e "${RED}✗ ERROR: no @teambit/harmony pin found in ${WORKSPACE_FILE}${NC}" | ||
| exit 1 | ||
| fi |
There was a problem hiding this comment.
1. Set -e aborts counting 🐞 Bug ☼ Reliability
In check-harmony-version-sync.sh, WS_COUNT is computed using grep -c . while set -e is enabled; when WS_VERSIONS is empty (e.g. the grep patterns don’t match), grep exits non-zero and the script terminates before reaching the explicit “no @teambit/harmony pin found” error handling. This makes the new CI step fail with less actionable output than intended for that edge case.
Agent Prompt
### Issue description
`scripts/check-harmony-version-sync.sh` enables `set -e` and then uses `grep -c .` to count lines in `WS_VERSIONS`. When `WS_VERSIONS` is empty (no matches), `grep -c` returns exit code 1, which can abort the script before it prints the intended explicit error message and guidance.
### Issue Context
This script is executed in CI (CircleCI job `check_env_cache_sync`) to validate harmony version synchronization.
### Fix Focus Areas
- scripts/check-harmony-version-sync.sh[16-47]
### Suggested fix
Move the `WS_COUNT` computation after the `-z "$WS_VERSIONS"` check, and/or replace the `grep -c .` counting with a command that does not fail on empty input. For example:
- After verifying `WS_VERSIONS` is non-empty, use `wc -l`:
- `WS_COUNT=$(printf '%s\n' "$WS_VERSIONS" | wc -l | tr -d ' ')`
Or keep `grep` but neutralize its exit code:
- `WS_COUNT=$(echo "$WS_VERSIONS" | grep -c . || true)`
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
|
Code review by qodo was updated up to the latest commit e751234 |
…an unversioned env When 'bit ci pr' syncs config from main and the lane still uses a workspace env (so envStrategy keeps the current env) while main migrated the component onto a different external env, the generic aspect merge copied main's teambit.envs/envs config verbatim. That config carries the env id without its version, so the snap failed with ExternalEnvWithoutVersion. Exclude teambit.envs/envs from the generic merge so envStrategy is the sole authority on envs.
Comment on lines
+74
to
+80
| // don't try to merge builder, it's possible that at one end it wasn't built yet, so it's empty. | ||
| // teambit.envs/envs is handled exclusively by envStrategy() — the generic aspect merge would copy | ||
| // `config.env` verbatim, which never carries the env's version (the version lives in the separate | ||
| // env-aspect entry that only envStrategy knows to attach). If envStrategy declines (e.g. the | ||
| // current env is a workspace component), letting the generic merge run would leak an unversioned | ||
| // external env and break the snap with ExternalEnvWithoutVersion. | ||
| private handledExtIds: string[] = [BuilderAspect.id, EnvsAspect.id]; |
There was a problem hiding this comment.
1. Broken env lane check 🐞 Bug ≡ Correctness
After adding EnvsAspect.id to handledExtIds, envStrategy() becomes the only path that can merge teambit.envs/envs. envStrategy() calls `isIdInWorkspaceOrOtherLane(this.currentEnv.id, this.otherEnv.version), but populateEnvs() derives otherEnv.version` from a different extension id than currentEnv.id, so the otherLaneIdsStr.includes(id@version) check can fail to detect that the current env exists in the other lane and incorrectly proceed to merge/sync env config.
Agent Prompt
### Issue description
`ComponentConfigMerger` now skips generic merging of `teambit.envs/envs` by seeding `handledExtIds` with `EnvsAspect.id`. This makes `envStrategy()` the sole source of a merge result for `EnvsAspect.id`.
However, `envStrategy()` currently calls:
```ts
this.isIdInWorkspaceOrOtherLane(this.currentEnv.id, this.otherEnv.version)
```
`populateEnvs()` computes `currentEnv` and `otherEnv` (and their versions) from different extension IDs, so when the env IDs differ, `otherEnv.version` does not correspond to `currentEnv.id`. Since `isIdInWorkspaceOrOtherLane()` checks `otherLaneIdsStr.includes(`${id}@${versionOnOtherLane}`)`, this makes the “other lane” portion of the condition unreliable.
### Issue Context
This can cause `envStrategy()` to fall through to `basicConfigMerge()` and emit an env merge result when it should have returned `null` (i.e., keep the current env because it exists in the other lane).
### Fix Focus Areas
- scopes/workspace/config-merger/component-config-merger.ts[196-217]
- scopes/workspace/config-merger/component-config-merger.ts[157-186]
- scopes/workspace/config-merger/component-config-merger.ts[631-633]
### Suggested fix
Adjust the check so the version used matches the ID being checked, e.g.:
- Pass `this.currentEnv.version` when checking `this.currentEnv.id`, **or**
- Change the helper to support an id-only check against `otherLaneIdsStr` (ignore version) for this env-specific branch.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
|
Code review by qodo was updated up to the latest commit 6101f4d |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Preparatory changes for the TypeScript 6 upgrade.
@teambit/harmonyto0.4.12, which points itsexportstypesconditions at the compiled.d.tsinstead of source. UndermoduleResolution: bundler/node16the old source-pointing types made harmony resolve to its shipped source and collide with its dist in the same program (TS2345source-vs-dist duplicate identity). Classicnoderesolution ignoresexports, so it's unaffected.tar: switch toimport * as tar— no default export under TS6's stricter ESM interop./// <reference types="chai-fs" />where the matchers are used — TS6 no longer auto-loads ambient@types.Formatin vizgraph,useWorkspacereturn type) for stricter TS6 inference.