Skip to content

Conversation

fdrab
Copy link
Contributor

@fdrab fdrab commented Jul 21, 2025

Changes made:

1, removed password field type as it no longer serves any purpose. only the base primitive types are left (string / number / int...)
2, added a wrapper

block around inputs to display a visibility toggle button to align centrally
3, secrets are now obfuscated using -webkit-text-security and this can be toggled by clicking on an "eye" icon.
4, if a field is marked as secret and the secret is not shown via visibility toggle, spellcheck is disabled
5, if input is secret, validation returns "*".repeat(v.length) instead of the value for types that return value in the error message
6, moved the "eye" icon a bit to the right to not overlap with the scrollbar

Feedback is welcome, I'm not a very good developer and I may not know all the things there is to know and may have missed something. These changes were tested in firefox / Edge and Chrome.

My 'gulp test' fails due to binding issues (TypeError: bind EINVAL 0.0.0.0). If anyone knows how to get around that, I'm all ears.

fdrab added 2 commits July 21, 2025 19:29
visibility toggle a bit more to the left
to not overlap with the scroll bar
@nzlosh
Copy link
Contributor

nzlosh commented Jul 22, 2025

The gulp looks like it might be related to an out of date web browser. Care to take a shot at updating the tests?

Browserslist: caniuse-lite is outdated. Please run:
  npx update-browserslist-db@latest
  Why you should do it regularly: https://github.com/browserslist/update-db#readme
/home/circleci/project/node_modules/react-textarea-autosize/dist/react-textarea-autosize.cjs.js:16
var isIE =  _isBrowser ? !!document.documentElement.currentStyle : false;

@nzlosh nzlosh requested a review from a team July 22, 2025 14:45
@fdrab
Copy link
Contributor Author

fdrab commented Jul 22, 2025

The gulp looks like it might be related to an out of date web browser. Care to take a shot at updating the tests?

Browserslist: caniuse-lite is outdated. Please run:
  npx update-browserslist-db@latest
  Why you should do it regularly: https://github.com/browserslist/update-db#readme
/home/circleci/project/node_modules/react-textarea-autosize/dist/react-textarea-autosize.cjs.js:16
var isIE =  _isBrowser ? !!document.documentElement.currentStyle : false;

will take a look

@fdrab
Copy link
Contributor Author

fdrab commented Jul 22, 2025

So I've checked some things and I'm starting to doubt it has anything to do with browser version or whatnot:
1, when I run the command that failed (npm run test-unit) in my virtualbox, all 308 tests pass
2, I've run the above mentioned command to update the browserslist and even though it was reported as success, the yarn.lock file wasn't updated with proper versions, so it still thinks that it's old, for some reason.
3, my subsequent PR for sanitizing the preview in actions succeeded, which suggests that there is no issue with browser or tests as they are (perhaps except for the fact that zombie is no longer updated and is an archived repo)

I'll try to take a deeper dive into this tomorrow or in the near future.

@fdrab
Copy link
Contributor Author

fdrab commented Jul 26, 2025

I'm unable to replicate the error in my environment. I've spun up a completely fresh install of Ubuntu 22.04, installed MongoDB, installed Redis / RabbitMQ, deployed ST2 3.9 unstable, cloned the repo, switched to the branch, ran all the commands required to do "gulp test" and everything works. Both unit tests and functional tests both work fine, no failures, 308 unit tests passed, 84 functional tests passed.

How can I get access to replicate the CircleCI run locally? Do I need to be part of the org? Haven't work with CircleCI before.

I want to be able to replicate the complete circleCI run on my machine using CircleCI tools without spinning up an EC2. There has to be something I can't see on my virtual machine.

@nzlosh
Copy link
Contributor

nzlosh commented Jul 26, 2025

The circleci yaml https://github.com/StackStorm/st2web/blob/master/.circleci/config.yml describes the process used to test on CircleCI. If you look at the build logs on CircleCI for the commit that failed you can see how the environment is composed, e.g. linux distribution, the environment variables, nodejs version etc.

the "show secrets" button works
@fdrab
Copy link
Contributor Author

fdrab commented Jul 30, 2025

😎

@cognifloyd cognifloyd added this to the 3.9.0 milestone Aug 12, 2025
Copy link
Member

@cognifloyd cognifloyd left a comment

Choose a reason for hiding this comment

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

Sorry for the delayed review. Just a couple of questions before I approve.

  1. Would you please post screenshots of what the secret fields look like with this change?

  2. Does the visibility button appear on all fields or just on secret fields?

Aside: This will probably fix other issues I've had with secrets fields, like the browser trying to autofill my st2 user+password when running an action. Exciting!

@fdrab
Copy link
Contributor Author

fdrab commented Aug 13, 2025

test flow with 2 inputs, string and an object, single line, hidden:
image

visible:
image

Multi-line input secret (only strings are multi-line, JSONs are parsed into single lines):
image

visible:
image

The string has a default value that's not masked, because frankly that shouldn't even be allowed, as it makes the secrets visible in plaintext in the yaml.

However, now that I made these screenshots, I've noticed that if the string/JSON is very long, it overlaps with the "eye" icon, so I have to fix that.

@fdrab
Copy link
Contributor Author

fdrab commented Aug 13, 2025

Fixed the overlap by adjusting padding from the right side, so that if the string or json is too long, it'll word-wrap before it hits the icon:
image

by increasing the padding from the
right side to 36px
Copy link
Member

@cognifloyd cognifloyd left a comment

Choose a reason for hiding this comment

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

Sweet. This should be a big improvement.

@fdrab
Copy link
Contributor Author

fdrab commented Aug 14, 2025

before this is merged please let me play around with the icon positioning. it being off-center irks me.

increased max-height of textarea to 100px and increased
padding from the right side to 40px from 36px to appear less crowded
@fdrab
Copy link
Contributor Author

fdrab commented Aug 16, 2025

OK I've added a wrapper around secret fields to center the icons and also increased the padding from the right side to 40px from 36px and increased max height to 100px from 50:

Hidden:
image

Not hidden:
image

Copy link
Member

@cognifloyd cognifloyd left a comment

Choose a reason for hiding this comment

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

LGTM. Are you ready for me to merge this?

@fdrab
Copy link
Contributor Author

fdrab commented Aug 16, 2025

Yes. Nothing for me to add to this one.

@cognifloyd cognifloyd merged commit 37accec into StackStorm:master Aug 17, 2025
3 checks passed
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.

3 participants