Solve cntrl+f issue on KVv2 JSON details and edit views #28808

Monkeychip · 2024-10-30T18:00:27Z

ent tests pass

Description

Address issue #28053 and closes PR #28604.

Replace the codemirror component on the KVv2 details page with the Hds::CodeBlock component. This solves the original issue, allowing the user to browser search the full secret while also moving us closer to HDS adoption.
For the edit view, this problem is solved by correctly taking advantage of the viewportMargin param provided by CodeMirror.

Notes:

For KVv2 only I calculate the viewportMargin based on the number of JSON object keys. I use the higher of the two numbers 10 or key count, but no higher than 1,000. Before we would also be using 10 (the default).
For the use cases where we were passing in "Infinity" this has negative performance implications and the only upside to us me three years ago misspelling viewportMargin 🫣.

viewportmargin.mov

github-actions · 2024-10-30T18:21:16Z

CI Results:
All Go tests succeeded! ✅

karabanov · 2024-10-31T14:34:15Z

I agree that this function may not be needed by everyone, but maybe it is possible to somehow make it permanently enabled for those who need it? That is, so that when you press this button, its state is remembered, for example, somewhere in the LocalStorage and remains such that you do not have to press the button every time

Monkeychip · 2024-10-31T15:16:07Z

@karabanov thank you so much for the feedback. We're running into an issue with adding this functionality to our current component and are looking at replace it with a HDS::codeblock component. The HDS codeblock allows you to cntrl + f inside a fixed height container so that you can search a whole secret. Would this solve you're problem, or is there still a desire to see the whole secret with the expand button? See screenshare of the new component below.

Re: localStorage, that's a potential option, but in general we try to minimize our use of localStorage. We've had similar requests for things like pinning an item to the top of a list, and other config-type requests. I'll throw this by the team, but no promises.

search-hds.mov

karabanov · 2024-11-01T19:26:10Z

@Monkeychip, I think if the ctrl+f search works correctly, then my colleagues and I will be quite satisfied.

Please tell me, will the search work in edit mode if you use the HDS::codeblock component? The fact is that if you stretch the codemirror window, the search will work both in viewing and editing mode

Monkeychip · 2024-11-01T21:45:48Z

@karabanov the Hds::Codeblock would only be for the details view not the edit as it's not designed to be a code editor. So this would solve the details view but not edit. I can make/push this PR to address the details view but for edit, that is a longer process and will involved doing some modifications to our current code editor (if possible). I'll go ahead and make a ticket for it and see if it's something we can address in another pr.

karabanov · 2024-11-02T09:34:31Z

@Monkeychip A working search by ctrl+f is also needed in the editing mode, since the JSON can be large and it is problematic to find the necessary key in it with your eyes to edit it, in addition, the keys are sorted in an unnatural order, which complicates the task even more

For example, if you do not switch to the JSON editing mode, all the keys will be visible on the page at once, they can be found by searching both in the viewing mode and in the editing mode, so why not just stretch the text field by 100% in the JSON editing mode so that all the keys become visible? This will be very similar to the normal editing mode, when all the keys are visible and the problem with searching will be solved.

The improvement that I propose in PR #28604 is to add one line and this will not require replacing the text editor, while solving the problem with searching for keys and displaying the content. This will even make the JSON editing mode more similar to the normal mode, since all the keys will be visible at once

How do you like this solution?

Monkeychip · 2024-11-04T21:59:42Z

@karabanov All very fair points. I went back to debugging why our current code editor does not allow searching beyond the viewport and got to the heart of the problem. We were incorrectly using a param called viewportMargin. This param tells the json editor to render the number of lines beyond what is visible in the viewport. A quick summary: it was defaulting to rendering 10 lines beyond the viewport, which for longer secrets is not enough. I amended this so the viewportMargin is dynamically calculated based on the size of the secret.

In conclusion:

Short term fix: Replace details view with new Hds::CodeBlock component that allows search. Update edit view to dynamical render the full secret which will effectively allow you to search the whole secret. No expand button/no change in default height (KVv2 is widely used and one change can cause lots of complaints so we try to avoid any functionality/layout changes unless absolutely necessary).
Longer term fix: We're working on getting a new code editor that will solve this problem and possibly include an expand button for the edit and readOnly views because you make a good argument for it and I put a request into the design team —thank you!

karabanov · 2024-11-05T12:00:00Z

@Monkeychip Thank you very much
I look forward to these changes being released soon

Monkeychip · 2024-11-06T22:30:12Z

ui/app/templates/components/console/log-json.hbs

@@ -8,7 +8,8 @@
    @showToolbar={{false}}
    @value={{stringify this.content}}
    @readOnly={{true}}
-    @viewportMargin="Infinity"
+    {{! ideally we calculate the "height" of the json data, but 100 should cover most cases }}


previously this was defaulting to 10, so another option is to remove this all together and let it default to 10 until someone complains?

If 100 doesn't have any major performance implications I think that should be okay here. I'm not sure what the data this typically renders - so it's possible 10 may be more reasonable

From what I can gather this is output for the ui console/terminal window. 10 for most situations should be enough unless they're trying to read a really long secret.

Is 100 going to have any performance implications? not that I can tell via testing, but also there's no documentation to say otherwise. From what I gather, they're cautioning against using "Infinity" because that could be bad if your json was millions of lines long.

Monkeychip · 2024-11-06T22:30:23Z

ui/app/templates/components/control-group-success.hbs

@@ -21,7 +21,8 @@
          @showToolbar={{false}}
          @value={{stringify this.unwrapData}}
          @readOnly={{true}}
-          @viewportMargin="Infinity"
+          {{! ideally we calculate the "height" of the json data, but 100 should cover most cases }}


same comment as above.

Monkeychip · 2024-11-06T22:41:53Z

ui/lib/kv/addon/components/kv-patch/subkeys-reveal.hbs

@@ -8,6 +8,6 @@
    <Hds::Text::Body @tag="p" @weight="semibold">Reveal subkeys in JSON</Hds::Text::Body>
  </Toggle>
  {{#if this.showSubkeys}}
-    <Hds::CodeBlock @value={{stringify @subkeys}} @hasLineNumbers={{false}} data-test-subkeys />
+    <Hds::CodeBlock @value={{stringify @subkeys}} @hasLineNumbers={{false}} data-test-code-block="subkeys" />


I updated all selectors added to the Hds::CodeBlock component so that we consistently used one GENERAL selector.

Monkeychip · 2024-11-06T22:43:28Z

ui/tests/acceptance/secrets/backend/kv/kv-v2-workflow-edge-cases-test.js

    );
    codemirror().setValue('{ "foo3": { "name": "bar3" } }');
    await click(FORM.saveBtn);

    // Details view
    await click(PAGE.secretTab('Secret'));
-    assert.dom(FORM.toggleJson).isNotDisabled();
-    assert.dom(FORM.toggleJson).isChecked();
-    assert.false(codemirror().getValue().includes('*'), 'Values are not obscured on details view');


A little clean up that was missed when we removed obscuring json values from the codemirror editor view PR #28261.

ui/tests/helpers/kv/kv-selectors.js

Monkeychip · 2024-11-06T22:58:14Z

ui/lib/kv/addon/components/kv-data-fields.hbs

+    </Hds::CodeBlock>
+  {{else}}
+    <JsonEditor
+      @title="{{if (eq @type 'create') 'Secret' 'Version'}} data"


Note: When you create a new version you see Version data vs Secret data.

github-actions · 2024-11-06T22:59:25Z

Build Results:
All builds succeeded! ✅

Monkeychip · 2024-11-06T23:03:51Z

ui/tests/helpers/kv/kv-selectors.js

@@ -90,8 +90,8 @@ export const PAGE = {
  },
  paths: {
    copyButton: (label) => `${PAGE.infoRowValue(label)} button`,


I go back and forth on putting this in the general selectors. Opted to keep as you see here until it becomes more obvious that these selectors will be used outside of kv.

Monkeychip · 2024-11-06T23:05:50Z

ui/tests/helpers/secret-engine/secret-engine-helpers.js

@@ -206,3 +206,49 @@ export const fillInAwsConfig = async (situation = 'withAccess') => {
    await fillIn(GENERAL.ttl.input('Identity token TTL'), '7200');
  }
 };
+


Admittedly, I got carried away on making this helper. I could have saved myself time and hardcoded or modified the KvDataFields defaults during a test situation. However, I can imagine this being a helpful function for future KV things. Gives us a quick way to make longer more complicated json objects. Also, it was kind of fun.

hellobontempo

Great work on this! Really thorough and nice communication with the reporter to get this resolved the right way.

Mostly just minor cleanup comments and questions. I'm curious about the accessibility change. It's end of day for me so feeling brain dead. Can circle back with an approval tomorrow ☺️

ui/lib/core/addon/components/json-editor.js

hellobontempo · 2024-11-07T00:56:04Z

ui/lib/kv/addon/components/kv-data-fields.hbs

-    @readOnly={{eq @type "details"}}
-  />
+  {{#if (eq @type "details")}}
+    <Hds::CodeBlock


😍 love seeing this component being used more!

hellobontempo · 2024-11-07T00:58:49Z

ui/app/templates/components/console/log-json.hbs

@@ -8,7 +8,8 @@
    @showToolbar={{false}}
    @value={{stringify this.content}}
    @readOnly={{true}}
-    @viewportMargin="Infinity"
+    {{! ideally we calculate the "height" of the json data, but 100 should cover most cases }}


If 100 doesn't have any major performance implications I think that should be okay here. I'm not sure what the data this typically renders - so it's possible 10 may be more reasonable

ui/lib/kv/addon/components/kv-data-fields.js

hellobontempo · 2024-11-07T01:50:42Z

ui/tests/integration/components/json-editor-test.js

    this.set('hashi-read-only-theme', 'hashi-read-only auto-height');
    setRunOptions({
      rules: {
        // CodeMirror has a secret textarea without a label that causes this problem
        label: { enabled: false },
        // TODO: investigate and fix Codemirror styling
        'color-contrast': { enabled: false },
+        // failing on .CodeMirror-scroll
+        'scrollable-region-focusable': { enabled: false },


Hm I wonder why this is failing now and wasn't before 🤔 is it worth looking into? I wonder if setting a default viewport margin value resolves this 💭

Good question. This is an ally testing thing—copied the same thing from Chelsea's fixes, here's one example but there are several more. From what I can tell it's because I'm testing just the component and now it scrolls.

hellobontempo · 2024-11-07T01:53:23Z

ui/lib/core/addon/components/json-editor.hbs

@@ -51,7 +51,7 @@
      mode=@mode
      readOnly=@readOnly
      theme=@theme
-      viewportMarg=@viewportMargin
+      viewportMargin=@viewportMargin


Should we set a default here?

The default is inherit from CodeMirror. If you don't pass viewportMargin it will default to 10, which is what it was doing when I incorrectly spelled the param.

Oh - is that default just builtin? In codemirror I see a default of an empty string (here) Maybe we could include that in the JSDOC below that it defaults to 10?

Will do, and good call out. I also modified the code-mirror line that you pointed out to explicitly state the default. Codemirror docs give this rec:

You should usually leave it at its default, 10

hellobontempo · 2024-11-07T01:55:30Z

ui/tests/integration/components/kv/kv-data-fields-test.js

+    setRunOptions({
+      rules: {
+        // failing on .CodeMirror-scroll
+        'scrollable-region-focusable': { enabled: false },


Same comment wondering why this is failing now

hellobontempo · 2024-11-07T01:57:00Z

ui/tests/integration/components/json-editor-test.js

@@ -129,4 +133,31 @@ module('Integration | Component | json-editor', function (hooks) {
      'even after hitting enter the value is still set correctly'
    );
  });
+
+  test('when @viewportMargin is not set user is unable to search a long secret', async function (assert) {


This test name feels a little confusing. Is there a default viewport margin and so the data is cut off so only a portion is rendered on the DOM? Or is there none at all so there's no scrolling?

Rewording to something like "no viewport margin renders only X (the default if we have one) lines of data on the DOM" feels a little more like it's explaining what's happening. Then in the test we can see we're testing by using ctrl+F

Co-authored-by: claire bontempo <[email protected]>

hellobontempo

Great work! Just a comment about mentioning the default viewportMargin in the jsdoc for JsonEditor

* initial changes need to add test coverage * change icon * replace original idea with hds::codeblock on kvv2 details view * changelog * fixing edit view by addressing viewportMargin * fix failing test * missedone * Update 28808.txt * Update json-editor.js * test coverage * update codeblock selector * Update general-selectors.ts * Update kv-data-fields.js * Update ui/lib/core/addon/components/json-editor.js Co-authored-by: claire bontempo <[email protected]> * Update ui/lib/kv/addon/components/kv-data-fields.js Co-authored-by: claire bontempo <[email protected]> * update test name * add default to modifier --------- Co-authored-by: claire bontempo <[email protected]>

Monkeychip added 2 commits October 30, 2024 11:42

initial changes need to add test coverage

b4b816f

change icon

f4776be

Monkeychip added ui backport/ent/1.16.x+ent Changes are backported to 1.16.x+ent backport/1.18.x backport/ent/1.17.x+ent Changes are backported to 1.17.x+ent labels Oct 30, 2024

github-actions bot added the hashicorp-contributed-pr If the PR is HashiCorp (i.e. not-community) contributed label Oct 30, 2024

Monkeychip added 2 commits November 1, 2024 13:41

replace original idea with hds::codeblock on kvv2 details view

e6090e3

changelog

5321c2e

Monkeychip added this to the 1.19.0-rc milestone Nov 1, 2024

Monkeychip changed the title ~~Add expand length btn on json codemirror~~ Solve cntrl+f issue on KVv2 details and edit Nov 4, 2024

Monkeychip changed the title ~~Solve cntrl+f issue on KVv2 details and edit~~ Solve cntrl+f issue on KVv2 JSOn details and edit views Nov 4, 2024

fixing edit view by addressing viewportMargin

d3ba3e9

Merge branch 'main' into ui/VAULT-33227/btn-codemirror-expand

6064b84

vercel bot deployed to Preview November 4, 2024 22:07 View deployment

Monkeychip added 2 commits November 4, 2024 17:55

fix failing test

1dfc2a2

missedone

6eeee82

Update 28808.txt

c9d5bb0