Skip to content

Grant actions card#3246

Open
cameronperera wants to merge 20 commits intollb/grants-builderfrom
grant-actions-card
Open

Grant actions card#3246
cameronperera wants to merge 20 commits intollb/grants-builderfrom
grant-actions-card

Conversation

@cameronperera
Copy link
Copy Markdown
Collaborator

@cameronperera cameronperera commented Apr 27, 2026

Description

🎟️ Jira ticket

Screenshots (if appropriate)

Screen.Recording.2026-04-27.at.3.09.06.PM.mov

How to Test

Visit the edit grants route. You should now see suggested actions on the right of the code editor. The suggestions should work as before and the suggested actions should match the actions in the completion window. Only difference is the actions card will not remove actions as they are used like the completion list.

Checklist

  • I have added before and after screenshots for UI changes
  • I have added JSON response output for API changes
  • I have added steps to reproduce and test for bug fixes in the description
  • I have commented on my code, particularly in hard-to-understand areas
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • I have added a11y-tests label to run a11y audit tests if needed

PCI review checklist

  • I have documented a clear reason for, and description of, the change I am making.
  • If applicable, I've documented a plan to revert these changes if they require more than reverting the pull request.
  • If applicable, I've documented the impact of any changes to security controls.
    Examples of changes to security controls include using new access control methods, adding or removing logging pipelines, etc.

lisbet-alvarez and others added 17 commits March 30, 2026 18:38
* chore: 🤖 Add initial page layout for code editor + sidebar

* test: 💍 add missing a11y ignores for exisitng color issue

✅ Closes: https://hashicorp.atlassian.net/browse/ICU-18460
* chore: 🤖 replace Rose::CodeEditor with Hds::CodeEditor

- Remove CodeEditor 5 dependency

* chore: 🤖 Replace CodeMirror5 dep

remove CodeMirror5 and replace existing editors with HDS::CodeEditor

* chore: 🤖 deleting old codeeditor styles

* feat: 🎸 changing code editor

changing code editor

✅ Closes: 0

* removed unused export

* pr cleanup

* updating tests for new code editor

* responding to PR feedback

* added whitespace

* fixing linting violations

* chore: 🤖 self-review PR

self-review PR in the course of switching to new HDS code editor

* fix: 🐛 fix linting bug

* fix: 🐛 unneeded dependency

remove jslint dependency as this is handled in the new code editor from
HDS

* fix: 🐛 Fixing import bug

Fix import bug caused by importing scss modules that no longer exist

* Fix and refactor code editor acceptance test

* Fix and refactor code editor acceptance test

* chore: 🤖 remove codemirror dependency

* refactor: 💡 fix merge conflict issues

* test: 💍 update e2e test selectors

* test: 💍 fix target spec click event

* refactor: 💡 clean up test selectors for code editor

* refactor: 💡 clean up storage bucket test selectors

* chore: 🤖 remove code mirror modifiers

---------

Co-authored-by: calcaide <ccorvo@hashicorp.com>
Co-authored-by: cameronperera <cameron.perera@hashicorp.com>
* feat: 🎸 Create string formats component

* Add missing copyright headers

* fix: 🐛 change to readonly

✅ Closes: https://hashicorp.atlassian.net/browse/ICU-18458
* feat: 🎸 add grant schema to mirage

* refactor: 💡 update grant schema mirage data

* refactor: 💡 fix route name in mirage
* feat: 🎸 add export options flyout

* fix: 🐛 update id to be ids

* test: 💍 add a11y ignores

* test: 💍 fix tests for grant string format

✅ Closes: https://hashicorp.atlassian.net/browse/ICU-18524
Co-authored-by: Zhihe Li <zhihe.li@hashicorp.com>
@cameronperera cameronperera self-assigned this Apr 27, 2026
@vercel
Copy link
Copy Markdown

vercel Bot commented Apr 27, 2026

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated (UTC)
boundary-ui Ready Ready Preview, Comment Apr 28, 2026 8:47pm
boundary-ui-desktop Ready Ready Preview, Comment Apr 28, 2026 8:47pm

Request Review

Copy link
Copy Markdown
Collaborator

@ZedLi ZedLi left a comment

Choose a reason for hiding this comment

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

Looks cool! Did an initial pass of some of the code

Comment on lines +63 to +65
if (update.selectionSet) {
const line = update.state.doc.lineAt(update.state.selection.main.head);
this.currentLineText = line.text;
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Should we just have all updating happen here and remove the onInput?

);

@tracked grantStringsText = (this.args.model?.grant_strings ?? []).join('\n');
@tracked currentLineText = this.args.model?.grant_strings?.[0] ?? '';
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Shouldn't we just default it to the first line when loading before there's any focus? Any reason we want to show nothing on first load?

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Should we be showing all actions on wildcard? We might want diverge from autocomplete to limit it to a different set in this case (maybe just default CRUDL like in figma?).

Also we should not make the card expand forever down and add scrolling if needed.

return getActionOptions(schema, typeValue, idsValue);
};

export const getDetectedResourceTypeForGrantLine = (
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Where is this used?

const hasEnteredActions = enteredActions.length > 0;
const options = filterByPrefix(
getActionOptions(schema, typeValue, idsValue),
getSuggestedActionsForGrantLine(schema, line.text),
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Why was this change necessary? We already parsed the grant line earlier and now it looks like we just re-parse the line again and then call getActionOptions again?

return (context) => grantCompletions(context, schema, translatedStrings);
};

export const createGrantLineHelpers = (grantsSchema) => {
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Is this needed here? It seems like it's a thin wrapper around the other exported functions. It also looks like you have a normalized schema in your component but still normalize the schema here.

};
};

const isNormalizedGrantsSchema = (grantsSchema) =>
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Why would we need this check?

parseGrantLine,
} from 'admin/utils/grant-completions';

const ACTIONS_WITH_RESOURCE_TYPE = new Set([
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

What is the purpose of this? Why do we just reflect back the resource type?

#grantLineHelpers = createGrantLineHelpers(this.args.grantsSchema ?? {});

get parsedGrantLine() {
return parseGrantLine(this.args.grantString);
Copy link
Copy Markdown
Collaborator

@ZedLi ZedLi Apr 28, 2026

Choose a reason for hiding this comment

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

This will parse the grant line every time this getter is called which seems inefficient. It looks like this can get called a lot of times the way we've structured these getters (over 10 times by my count). I don't think it's a realistic performance issue since parsing should be relatively quick but if it ever changes we'll get caught by this. It might be worth adding the cached decorator here.

Though I'm curious if this indicates a more structural issue. We have a lot of getters (which I find a bit hard to read). I wonder if most of this logic should have actually been exported in an object out of a util function (whether it's autocomplete or linting) which at least lets a reader to see the different variables and logic at once instead of jumping all over.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants