🇧🇬Bulgaria @valthebald

@lpeabody: I believe that would be another issue.
IMO, getting rid of thread ID in page HTML is something that should happen no matter which storage to use

@igor mashevskyi: note that there is also DeepChatFormBlock plugin (id="ai_deepchat_block"), and that one is used in CMS recipe, so probably the same changes should be done in block.settings.ai_deepchat_block.schema.yml

Categorizing

I checked the core, and see usage of enforced dependencies only in test modules (so, not merging MR yet). This won't make any harm though.
On the other side, having prompt reference in the config is the right thing to do (so RTBC).
Bottom line, I'll let this to decide for someone else :)

I reviewed related issue in ai_translate, this issue's MR follows the same logic.
Marking RTBC

Back to needs review after rebasing on the latest 1.2.x

@bbruno: I have fixed merged conflicts after rebasing on the last 1.2.x (there were less that I thought), and checked that the chat works in toolbar (anonymous and admin) and "regular" position (anonymous only because of permissions)

@svendecabooter: oops, replacing the prompt text is a good catch :)
regarding enforced dependency - isn't it added automatically by core?

Merged to 1.2.x, thank you @svendecabooter!

I checked fresh install, as well as update from 1.2.x-dev, translations work as expected.
I have some thoughts about optimizing prompt selection UI, but this is definitely not a release blocker.

I tried a fresh reinstall of ai from the latest 1.2, and then switched to the feature branch.
This revealed another issue - prompt type should be installed (if not exists) in hook_update as well. Otherwise there is
"No AI Prompt Types exist. These are normally provided by modules within the AI Ecosystem" error when trying to edit a prompt

Update hook worked like a charm, thank you @svendecabooter!
No strong opinion here, but do we need to keep both default and modified prompts? In the end, we have the default one in module's install folder for the reference

That greatly depends on the target release date for 2.0 and planned features that haven't found their way into 1.2.0.
There is sense in 1.3.x if all the following is true:

1. We can't do all the refactoring/basing on symfony components in ~3-4 months
2. Within the same 3-4 months period, there features that are an easy and/or big win, but break BC

LGTM

valthebald → created an issue.

Merged to 1.2.x, thanks @nikro!

@marcus_johansson: Core has https://www.drupal.org/about/core/policies/roles-and-responsibilities/pa... → exactly for that - pay respects to people who were maintainers, but have stepped down

Took a bit longer than 30 minutes, but PoC is fully working (see the MR). Summarized changes:

1. Remove thread_id from page' drupalSettings
2. Removed thread setting logic from DeepChatApi::setAssistant() (good that there is already DeepChatApi::setThreadsId()
3. Move historicalMessages() method from DeepChatFormBlock to DeepChatApi (because history is AJAX-loaded)
4. Create JS class deepChatSession that handles all the logic of initialization/retrieving the history/resetting etc.
5. Every instance of deepChatElement in deepchat-init.js gets an instance of deepChatSession
6. Changed input and output parameters of DeepChatApi and ResetSession controllers

Open issues/questions

1. Current approach (before or after switch to local storage) assumes there is only one chat session on the page. In theory, it is possible to pass unique storage ids to deepChatSession instances, thus working with different assistants/thread. Is this needed?
2. Is there a need in anonymous session protection other than random 20-character string? Maybe store also user ID, to protect authenticated sessions from hijacking? Can currently unused deepChatSession.secret variable be of any value?
3. Should the history be stored in local storage (all of it or limited number of recent messages)?

valthebald → created an issue.

@svendecabooter: thank you, merged!

valthebald → created an issue.

Here's the concept (in MR 907):

1. Bind thread_id to a random secret string, stored on client side
2. Do not store thread_id in drupalSettings (cache killer!). Instead, use chatSettings JS object, serialized to browser's storage
3. When there is no thread_id in chatSettings, call getSession() to generate fresh one.

The code in its current condition works and doesn't start a session.
What's missing is security check (need to prevent session hijacking)

Changing back to Needs review for wider visibility

LGTM as well

There are some merge conflicts, I wonder if it'd be easier to open a new MR or fix them?

@lpeabody:

Just to be clear, a session cannot exist at all if the page cache is to be used.

Totally, that's why session/local storage accompanied by expirable key/value storage

I will push to the issue branch later today

@svendecabooter: I was wondering the same question!
While I don't know what is the "desired" behavior, the reason I decided to go with the OR logic is that it seems more flexible, i.e. you may want to give certain roles permission to only create translations using the core flow, and some other roles to only create translations using AI.

As an alternative to browser storage (local or session), here's another trick that was used in a semi-decoupled Drupal project: the whole conversation is stored on server side (as it is today), and there's conversation id that is stored in session/local storage of the browser. It is also possible to protect the conversation with randomly generated secret, stored by KeyValueExpirableFactory service.
That way:

• Nothing is stored in the session, so page cache is not broken
• Conversation is protected with a mechanism similar to session generation in Drupal
• Conversation itself is not stored on client side, so lower chance to face clientStorage limitations

Closing as outdated, no evidence this bug exists in 1.2.x, and no feedback from original reporter

This fix accompanies the one from ✨ AI Translate Module Leaves Fields Empty When No Translation is Found Active .
I have search the code base and don't see any calls to addTranslation() that don't get values from original entity.
Thanks everyone, merged to 1.2.x!

Starting review

@mkalkbrenner incubator module is a good mode, reminds me of solrlog that found its way into searchapi_solr :)

After a quick check of https://git.drupalcode.org/project/permissions_by_term/-/tree/3.1.x-dev/..., there's no file AutomatorPluginDeriver.php
Which version of permission_by_term do you use? Any chance there was local override?

Setting the component

@svendecabooter: it's always a tricky question for contrib-contrib plugins :)
I personally don't have a strong opinion here

PHPCS reports some code style issues, please check https://git.drupalcode.org/issue/ai-3541883/-/jobs/6569528

LGTM and the test bot is happy

valthebald → created an issue.

valthebald → created an issue.

valthebald → created an issue.

Option to set the default state for the new translation was already added

@anmolgoyal74: that's a good proposal! Do you want to create a merge request, or someone else can?

I've reworked original idea, and added the logic to hide "AI translations" column when user doesn't have permissions or all languages are already translated

valthebald → changed the visibility of the branch 1.0.x to hidden.

I have added "beta blocker" tag to all the mentioned issues, feel free to add tag/untag/add more issues

valthebald → created an issue.

Bumping the version (again)

Merged into 1.2.x, thanks everyone!

@yuri.p: common flow is to target the current default branch (1.2.x), and then, if needed, backport to previous one (1.1.x)
There's almost no chance new commits will be added to 1.0.x

@IliaNoz: any reason you changed the target version from 1.2.x to 1.1.x?

@IliaNoz: any reason you changed the target version from 1.2.x to 1.1.x?

Lowering down the version, since we already have enough issues in the work, and it would be good for people to understand which issues go where

valthebald → created an issue.

valthebald → created an issue.

This is fixed in ✨ Content moderation options in entity translations Active . If you want this feature to be 1.1.x, please provide a backport

valthebald → created an issue.

valthebald → created an issue.