-
Notifications
You must be signed in to change notification settings - Fork 107
[v12] feat(ui-tree-browser): TreeBrowser rework #2342
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: v12
Are you sure you want to change the base?
Conversation
|
|
||
| function bootstrap() { | ||
| execSync(path.resolve('scripts/clean.js'), opts) |
Check warning
Code scanning / CodeQL
Shell command built from environment values
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 12 days ago
In general, to fix this class of problem you should avoid passing a single command string to child_process.execSync when any part of that string is dynamic. Instead, either (1) use execFileSync with the executable and its arguments passed as separate parameters, or (2) if you need to run a Node script, invoke process.execPath (the Node binary) with the script path as an argument, again using an argument array. This way, the shell is not involved and dynamic values are not interpreted as shell syntax.
For this file, the best fix is to change the bootstrap function so it no longer passes the resolved script path as a shell command string. A simple, behavior-preserving change is:
- Call
execFileSyncinstead ofexecSync. - Use
process.execPathas the command (the current Node executable). - Pass
[path.resolve('scripts/clean.js')]as the argument array. - Keep
optsunchanged sostdio: 'inherit'behavior is preserved.
We also need to import execFileSync from child_process alongside the existing execSync and fork imports. All changes are confined to scripts/bootstrap.js:
- At the top, extend the destructuring require to include
execFileSync. - In
bootstrap(), replaceexecSync(path.resolve('scripts/clean.js'), opts)withexecFileSync(process.execPath, [path.resolve('scripts/clean.js')], opts).
-
Copy modified line R27 -
Copy modified line R68
| @@ -24,7 +24,7 @@ | ||
| * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE | ||
| * SOFTWARE. | ||
| */ | ||
| const { execSync, fork } = require('child_process') | ||
| const { execSync, execFileSync, fork } = require('child_process') | ||
| const path = require('path') | ||
|
|
||
| const opts = { stdio: 'inherit' } | ||
| @@ -65,7 +65,7 @@ | ||
| } | ||
|
|
||
| function bootstrap() { | ||
| execSync(path.resolve('scripts/clean.js'), opts) | ||
| execFileSync(process.execPath, [path.resolve('scripts/clean.js')], opts) | ||
| buildProject() | ||
| } | ||
|
|
6abd48e to
ee0bed7
Compare
|
ee0bed7 to
8f81f6f
Compare
matyasf
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you also update the example in the regression test to use the new icons?
| const iconElement = isValidElement(icon) | ||
| ? safeCloneElement(icon, { size, color: iconColor }) | ||
| : callRenderProp(icon, { size, color: iconColor }) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should come up with a pattern for this... @joyenjoyer FYI
b8e26a4 to
ae7c1ea
Compare
ae7c1ea to
e394b93
Compare
matyasf
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks good!
ToMESSKa
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
see my comments
| - `iconsMarginRightSmall` (new) | ||
| - `iconsMarginRightMedium` (new) | ||
| - `iconsMarginRightLarge` (new) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are we supposed to put the new tokens into the upgrade guide? I think only the renamed or removed ones are relevant?
e394b93 to
ef34995
Compare






INSTUI-4818
Summary
Migrated TreeBrowser component from the old theming system.
Test plan
On the documentation page, verify that everything displays and works correctly.
Co-Authored-By: 🤖 Claude