Add space and move the cursor after drag and drop to empty prompt, ch… #387

yzhangok · 2025-07-02T06:32:44Z

…ange image icon and improve overlay removal's latency

Problem

This is to fix the issue found during bug bash

The image icon looks different from other icon
The latency of the removal of drag overlay is high
When prompt input is empty, context added by drag&drop is not separated by empty space, and the cursor is not moving to the end of the input
The images added by drag and drop sometimes are added to pinned context

Solution

Use the right icon

Force repaint before dispatch the event
The insertContextToPosition is not able to handle the case when cursor position is 0, this is only happen in drag&drop, since customer does not need to type/click to trigger add context.
This is reproducable when first click on @Pin Context, then does not select anything. Since the topBarTitleClick is still true in this case, the context added through drag&drop will be added to pinned context. Dispatch the event RESET_TOP_BAR_CLICKED (Renamed from CONTEXT_INSERTED) to reset the trigger source before add the context.

Tests

[ x ] I have tested this change on VSCode
I have tested this change on JetBrains
I have added/updated the documentation (if applicable)

License

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

laileni-aws

LGTM but this image looks odd to me in src/components/icon/icons/image.svg but looks good in PR description though.

One more observation: Icon is little up compared to Icon title in below screenshot.

src/static.ts

avi-alpert · 2025-07-03T12:58:39Z

It seems like the drag & drop functionality in demo app is broken, this makes it hard for others to validate the functionality of this feature

Screen.Recording.2025-07-03.at.8.57.27.AM.mov

dogusata

I'm not fan of strictly defining the whole drag drop functionality just for images. This will cause duplication of code whenever we need different file types to be handled. However, you can do note it as a future TODO.

However, please avoid using hard coded strings for check statements. It is pretty open to mistakes. And not future proof. Please move the image string comparison to type. in the QuickActionCommand type definition command can be string | ImageCommandTypes which you can define. Then the consumer has to follow it.

dogusata · 2025-07-08T10:00:58Z

src/components/chat-item/chat-wrapper.ts

+    MynahUITabsStore.getInstance().addListenerToDataStore(props.tabId, 'contextCommands', (contextCommands: QuickActionCommandGroup[]) => {
+      // Feature flag for image context command
+      this.imageContextFeatureEnabled = contextCommands?.some(group =>
+        group.commands.some((cmd: QuickActionCommand) => cmd.command.toLowerCase() === 'image')


Still not fan of using static text comparisons, please avoid doing it.

Define file type list if possible, since you can make the same thing for different file types in the future. I'm pretty sure that it will not be limited to image files in close future.

If possible, grab them from config with their icons.

This is the feature flags, will be removed after image context is launched in all IDEs.

The IDE controls image-related functionality through the imageContextEnabled feature flag. When this flag is set to true, the language server adds the Image option to the available context types.

Users can add images to the context in Mynah UI through three methods:

Using the context command menu

Typing the @image: command

Dragging and dropping images
To maintain consistency, we've implemented a centralized feature flag that controls the visibility of all three image-adding methods. This ensures that image functionality is either entirely available or unavailable across for an IDE.

There is no imageContextEnabled feature flag thats controllable by a consumer, from what I can see in this PR you still depend on filtering contextCommands to see if image is in there, and thats how you set imageContextEnabled

dogusata · 2025-07-08T10:03:30Z

src/components/chat-item/chat-wrapper.ts

@@ -193,72 +220,27 @@ export class ChatWrapper {
      persistent: true,
      events: {
        dragenter: (e: DragEvent) => {
-          if (!this.hasImageContextCommand()) return;
+          if (!this.imageContextFeatureEnabled) return;


Sill don't understand why we're strictly defining this feature for image, I still don't see it as a future proof implementation. What if we want to have more files in the future? Will we define everything one by one?

avi-alpert · 2025-07-08T16:54:15Z

src/components/chat-item/chat-wrapper.ts

+      classNames: [ 'mynah-drag-overlay-content' ],
+      children: [
+        new Icon({ icon: MynahIcons.IMAGE }).render,
+        { type: 'span', children: [ 'Add image to context' ] }


no text should be hardcoded, the string and icon should come from config, as Dogus pointed out in other comments

hardcoding it here means that the consumer has no control over what appears here. mynah-ui changes will be needed every time we want to change the string

…ange image icon and improve overlay removal's latency

Fix typo Co-authored-by: Avi Alpert <[email protected]>

avi-alpert · 2025-07-08T20:48:08Z

src/static.ts

@@ -789,6 +790,7 @@ export interface ConfigOptions {
  codeInsertToCursorEnabled?: boolean;
  codeCopyToClipboardEnabled?: boolean;
  test?: boolean;
+  dragOverlayIcon?: MynahIcons | MynahIconsType | CustomIcon;


TODO: Please add documentation to CONFIG.md for these new fields

Will send followup pr to fix this

yeah thats fine

avi-alpert · 2025-07-08T21:01:13Z

src/components/chat-item/chat-wrapper.ts

@@ -66,6 +71,26 @@ export class ChatWrapper {
      type: 'div',
      classNames: [ 'mynah-chat-wrapper-header-spacer' ]
    });
+
+    /*
+    The IDE controls image-related functionality through the imageContextEnabled feature flag.


This comment is not true. You did not add an imageContextEnabled feature flag that the IDE can control. This PR sets imageContextEnabled based on whether an image command is included in the contextCommands

Please either fix the comment or preferably, fix the functionality and have it as a feature flag set in config

avi-alpert · 2025-07-08T21:02:55Z

src/components/chat-item/chat-wrapper.ts

+              left: 0;
+              width: 100%;
+              height: 100%;
+              background: rgba(255,255,255,0.7);


hardcoding colors is not allowed, you need to use variables (an existing one or add your own)

There are several reasons why we don't hardcode colors in mynah-ui, the main one being is this may break on different themes

avi-alpert · 2025-07-08T21:03:46Z

src/components/chat-item/chat-wrapper.ts

+              flex-direction: column;
+              align-items: center;
+              z-index: 2;
+              font-size: 1.2em;


do not use hardcoded font size, use font size variable

avi-alpert · 2025-07-08T21:03:52Z

src/components/chat-item/chat-wrapper.ts

+              align-items: center;
+              z-index: 2;
+              font-size: 1.2em;
+              color: #333;


another hardcoded color

avi-alpert

I will approve to unblock launch - however please refer back to this PR's comments to be addressed in a follow up

yzhangok requested a review from a team as a code owner July 2, 2025 06:32

laileni-aws previously approved these changes Jul 2, 2025

View reviewed changes

avi-alpert reviewed Jul 3, 2025

View reviewed changes

src/static.ts Outdated Show resolved Hide resolved

yzhangok dismissed laileni-aws’s stale review via f696b4f July 3, 2025 17:47

dogusata requested changes Jul 8, 2025

View reviewed changes

avi-alpert reviewed Jul 8, 2025

View reviewed changes

yzhangok and others added 11 commits July 8, 2025 13:39

Add space and move the cursor after drag and drop to empty prompt, ch…

9aa8c3d

…ange image icon and improve overlay removal's latency

Rename the CONTEXT_INSERTED event to RESET_TOP_BAR_CLICKED

5e67cd2

change static function to private

cf04a8c

Update src/static.ts

2bd1549

Fix typo Co-authored-by: Avi Alpert <[email protected]>

add/remove overlay by controlling the css of the element

349658c

fix comment

2882744

add back cancelevent

a3bcc3e

unit test fail

e9ec13f

Add data store listener to set imageContextCommandEnabled

5f0f5f0

refactor comments

41121bd

refactor comments

b397970

yzhangok force-pushed the bugbag-fix branch from 614d202 to b397970 Compare July 8, 2025 20:41

avi-alpert reviewed Jul 8, 2025

View reviewed changes

refactor comments

8650bd3

avi-alpert reviewed Jul 8, 2025

View reviewed changes

avi-alpert approved these changes Jul 8, 2025

View reviewed changes

yzhangok merged commit d6df339 into aws:main Jul 8, 2025
4 of 5 checks passed

laileni-aws mentioned this pull request Jul 8, 2025

Bumping up mynah version to 4.35.8 #390

Merged

3 tasks

Add space and move the cursor after drag and drop to empty prompt, ch… #387

Add space and move the cursor after drag and drop to empty prompt, ch… #387

Conversation

yzhangok commented Jul 2, 2025

Problem

Solution

Tests

License

Uh oh!

laileni-aws left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

avi-alpert commented Jul 3, 2025

Uh oh!

dogusata left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

avi-alpert left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!