-
Notifications
You must be signed in to change notification settings - Fork 452
Label each field with which intent template that it is used in #5527
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: main
Are you sure you want to change the base?
Conversation
This is a great PR, and the feature is a valuable addition. One suggestion to improve the TypeScript implementation would be to refactor the For example, in export enum FeatureType {
Incubate = 0,
Existing = 1,
CodeChange = 2,
Deprecation = 3,
Enterprise = 4,
} This would provide several benefits:
This change would also allow for much stronger typing of related data structures. For instance, the export type FieldIntentUsage = Partial<Record<FeatureType, Set<IntentType>>>; This is a more robust and idiomatic TypeScript approach that would allow the compiler to catch any missing feature types in structures like This review was provided by Gemini CLI powered by GitHub MCP Server |
I was a little AI happy 😅 with the review. But let me take a look now lol |
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.
LGTM. Let me know your thoughts on the suggestions from AI and me.
} | ||
|
||
// Helper map to store details for each intent type. | ||
const INTENT_TYPE_DETAILS = { |
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.
Similar to the AI suggestion, you could do something like this to help with typing:
interface IntentTypeDetail {
abbreviation: string;
className: string;
title: string;
}
const INTENT_TYPE_DETAILS: Record<IntentType, IntentTypeDetail> = { ... };
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.
All of the above are great suggestions, and I've implemented them! 🙂
I have concerns about two aspects of this change:
What if we just put a brief notation in the field-level help text? E.g., for "Flag Name", it could be:
|
I do understand the concern here, but this has been a repeated pain point for multiple users. The information can be derived from looking at the intent templates, but that's the pain point that surfaces - users don't realize which fields get surfaced in the intent and which do not. For example, a user might add an important caveat in the "Comments" field rather than the summary field, but then not realize that now the intent email will not surface this caveat because the "Comments" field is not in the intent, so the intent feels like it is missing information. I think I'm of the opinion that, since all of this information is public, if a field is truly important for the launch process, it should just be surfaced in the intent as well. Obfuscating what fields are part of which intent in order to encourage completion is probably more of a symptom of intents not containing the information they should.
I definitely agree with this, and since this PR is using "PSA", three-letter icons are in the mix already. I can add this change 🙂
This gets murky with all of the conditional logic that we use in our intent generation. We would then need description modifiers for each feature type as well as intent type, for instance. It seems more difficult to maintain accurate descriptions and make intent changes in this case. |
The problem with labeling the fields that is used in intents is that it implies that the other fields that are not labeled with anything are not useful for anything. So, let's label everything. The
That would leave just a few fields unlabeled, but their purpose should be clear from the name, or in some cases they truly are rarely used, e.g., comments. |
I like this approach 🙂 Is there a place I can discern this mapping within our codebase? |
No, this would be making it explicit for the first time. If you specify |
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.
The change looks good. But some of the playwright snapshots look like there was a change to the main menu, maybe because of spacing. Was that expected?
d7ea67b
to
dc84549
Compare
For the main menu, I believe that the actual change is some form spacing, and the only thing that's not changing in spacing is the main menu - but this makes it look like the main menu is changing because the playwright test screenshot is centered around a specific location in the form. 😅 |
Fixes #5495
This change adds new icons next to fields that clarify which fields are used in which intent templates.
Note that I had to cross-reference our
intent_to_implement.html
file to ensure I was capturing which fields are available in which template, and I believe I'm close to correct in documenting the field usage in intents throughform-field-specs
. If certain fields are strangely marked as unused or not in certain templates, this is likely a flaw our current intent template.Example of changes:
Screen recording 2025-10-09 11.36.23 AM.webm