This should depend on css I did not remember details. At some moment I did some changes in dialog css but they were probably not used because the client was rewritten slightly. Although why this dialog should be there?

I agree the dialog probably should not be there. But if it does show, it should be much smaller.

Yes, we can set the absolute height in pixels within this method createOpenPopupDialog but it is also possible to change css in order that all dialogs adapt its height depending on its content.

The changes look fine, but the dialog is still pretty ugly :-). Why don't we use a Dojo dialog here? We already use it in other places.

Sergey Ivanovskiy wrote:

I can perform only constructive requirements dynamic height and so on.

Don't worry, I cannot do it any better either :-). That is why I think it makes sense to use Dojo.

Just search for dijit/Dialog.

The styles can be used to add border and other elements if there are needed. So it needs to have a template of look and feel.

Sergey Ivanovskiy wrote:

Hynek, it is not used in the web client but only in p2j.perf_tests.js although I did not remember but there was an issue when I tried to use this dialog with upload/download files dialog.

It is also used in the log stack dialog and report server.

Sergey Ivanovskiy wrote:

The styles can be used to add border and other elements if there are needed. So it needs to have a template of look and feel.

Yes, this can be certainly done. But why do we need different look from the other modal dialogs?

Hynek Cihlar wrote:

Sergey Ivanovskiy wrote:

The styles can be used to add border and other elements if there are needed. So it needs to have a template of look and feel.

Yes, this can be certainly done. But why do we need different look from the other modal dialogs?

Could you help to find a source task that I got at some moment in the past to complete the part of this logic
implemented in p2j.screen.js

me.chooseFiles = function(direct, uploadId, title, multiple, upload, filters) 

Sergey Ivanovskiy wrote:

I could not find an exact task where the obstacles to use dojo dialog was discussed. From the code the obstacles can be in interactions between dojo uploader and a modal dialog or/and integration of these components into our logic.

Can you clarify what obstacles do you mean? I used dijit/Dialog and never had any issues with that. You probably mean #5487.

Yes, dojo dialogs should not be the issue but there is no your review in this task #5487. So I did not recall.

This task to replace p2.ModalDialog can take more time. If there are no objections from Greg, then I can follow it. It needs to have two scenarios where selected files uploaded directly and these files are selected, correct?

Sergey Ivanovskiy wrote:

Yes, dojo dialogs should not be the issue but there is no your review in this task #5487. So I did not recall.

This task to replace p2.ModalDialog can take more time.

Why is that? The last time I used dijit/Dialog, it took me not more than an hour to have a workable modal dialog.

It needs to have two scenarios where selected files uploaded directly and these files are selected, correct?

I'm not sure how relvant is this for this task?

Hynek Cihlar wrote:

Sergey Ivanovskiy wrote:

Yes, dojo dialogs should not be the issue but there is no your review in this task #5487. So I did not recall.

This task to replace p2.ModalDialog can take more time.

Why is that? The last time I used dijit/Dialog, it took me not more than an hour to have a workable modal dialog.

It needs to have two scenarios where selected files uploaded directly and these files are selected, correct?

I'm not sure how relevant is this for this task?

The logic implemented in chooseFiles = function(direct, uploadId, title, multiple, upload, filters) is complex and two dialogs are used: the first one is to run upload as a user action and the second dialog shows uploading progress and possible errors. All these cases are needed to check and to test.

Sergey Ivanovskiy wrote:

The logic implemented in chooseFiles = function(direct, uploadId, title, multiple, upload, filters) is complex and two dialogs are used: the first one is to run upload as a user action and the second dialog shows uploading progress and possible errors. All these cases are needed to check and to test.

Sorry, I don't follow. This issue is about the connection dialog.

Now I am confused about what to do. Please explain what would need to do. There is a global task is to replace p2j.ModalDialog with dojo dialogs consistently. There are some p2j.ModalDialog dialogs: open popup dialog to permit windows.open in some cases, dialogs for upload functionality and dialogs to notify about connection issues.

If this task is to make auto height, then these changes are done. I recalled that text with white background without border is what we discussed.

Sergey Ivanovskiy wrote:

If this task is to make auto height, then these changes are done. I recalled that text with white background without border is what we discussed.

The goal of the issue is to make the reconnection dialog "less ugly". The height adjust was a solution picked. But my point was to make the dialog a dijit Dialog to make the appearance more elegant and consistent with the other dialogs in the app - file upload, stack trace dialog, report server, etc.

Hynek, why did you call this dialog reconnect? First, Greg posted an open resource dialog and the second argument is that we discussed all these dialogs before to have white rectangular background but dojo dialogs has different styles.

The third argument is that p2j.ModalDialog can have any styles but styles or templates are not specified. I do not think that restyling dojo dialogs is similarly simple as p2j.ModalDialog.

Sergey Ivanovskiy wrote:

Hynek, why did you call this dialog reconnect?

Sorry, I misread the actual dialog type. But I think the same may apply to the reconnect dialog: "It makes the system look unpolished/unfinished." as stated in the first note.

the second argument is that we discussed all these dialogs before to have white rectangular background but dojo dialogs has different styles.

It is certainly subjective, but I agree with Greg, that the simple rectangular appearances of the dialogs don't look appealing.

Sergey Ivanovskiy wrote:

The third argument is that p2j.ModalDialog can have any styles but styles or templates are not specified. I do not think that restyling dojo dialogs is similarly simple as p2j.ModalDialog.

Why would dojo styling be different? In the end it is still a set of CSS styles and some HTML structure. I'm not trying to sell dojo UI, frankly I think there are better UIs out there, but we already use it, so I think it makes sense to have a unified look and styling.

I understand reasons to use dojo dialog but in this case I need a template, because any dialog has title bar and buttons panel. p2j.ModalDialog has some listener logic implemented for this dialog because for this case there are several files downloaded asynchronously and only one dialog for these requests to windows.open.
I did not defend this implementation p2j.ModalDialog but I need some time to replace the logic if Greg agree with this task. I think that I would leave interfaces of p2j.ModalDialog but implement this dialog as dojo dialog. This idea looks more reasonable for me.

Sergey Ivanovskiy wrote:

I did not defend this implementation p2j.ModalDialog but I need some time to replace the logic if Greg agree with this task.

Sounds good.

I think that I would leave interfaces of p2j.ModalDialog but implement this dialog as dojo dialog. This idea looks more reasonable for me.

Good idea.

Hynek Cihlar wrote:

Sergey Ivanovskiy wrote:

I did not defend this implementation p2j.ModalDialog but I need some time to replace the logic if Greg agree with this task.

Yes, I don't want to have to redo this work every time I see an ugly dialog. Fix them all now.

OK. Following this plan #10915-31 in mind.

Please review the committed revision 16324 (10915a) that reused dijit.Dialog.

Committed rev 16326 (10915a) fixed the footer panel height. Planning to rebase and to add history comments and to clean debug code.

We have an issue in the keyboard that trigger SPACE 4GL key press and key release on keyup event. This affects the logic so the notification dialog to be closed on SPACE key down but SPACE keyup delivers the same keydown and up to the converted application, but the actual key down should be eaten by the modal user dialog (purposed to notify a user about uploading file).

Hynek, could you review the committed revision 16334 (10915a)? In the summary diff I missed history entries and committing them in rev 16335.

Code review 10915a. The changes look good I just checked in a copyright year update and a couple of formatting issues.

However please note that pressing Escape on the disconnection dialog will make it re-appear with no title bar.

Thank you for the review. It seems that it should be server unavailable dialog without title. Will check and fix it.

However please note that pressing Escape on the disconnection dialog will make it re-appear with no title bar.

No, I cannot reproduce this issue. May be you were in a hurry and did not note that this was server unavailable dialog without the title. The logic is in p2j.socket: doRedirectToLogoutPage.

Could you check this video, please? No view was recorded.

It is enough to do a print screen. How did you emulate restore connection dialog?

Sergey Ivanovskiy wrote:

It is enough to do a print screen. How did you emulate restore connection dialog?

A screen shot wouldn't show the transition. Just kill the server and do a user input (mouse click in the app for example).

Sergey Ivanovskiy wrote:

Could you check this video, please? No view was recorded.

Try a different player.

Hynek Cihlar wrote:

Sergey Ivanovskiy wrote:

It is enough to do a print screen. How did you emulate restore connection dialog?

A screen shot wouldn't show the transition. Just kill the server and do a user input (mouse click in the app for example).

I do not understand but I have no issues because I used the same scenarios. The server is unavailable what I can see on your video with VLC. You moved the dialog and pressed escape.

The server unavailable dialog has no title by its construction. Please see the code.

      serverUnavailableDialog = new p2j.ModalDialog("serverUnavailableDialog",
                                                    document.body,
                                                    serverUnavailableMessage + " " + p2j.pressEscape,
                                                    p2j.pressEscape,
                                                    true, {"noTitle" : true, "closable" : false});

Sergey Ivanovskiy wrote:

The server unavailable dialog has no title by its construction. Please see the code.

Should it have one?

This dialog has no title by the previous design. If you proposed to add title to this dialog, then what would be its title name? Please make your notes clear for me. The WebClient_nl_NL.json should be changed because of new translation.

Sergey Ivanovskiy wrote:

This dialog has no title by the previous design. If you proposed to add title to this dialog, then what would be its title name? Please make your notes clear for me. The WebClient_nl_NL.json should be changed because of new translation.

All the modal dialogs in the app have a title. The title clearly communicates to the end-user what is going on. If the title says "Server Unavailable", the user that already saw the dialog in the past doesn't need to scan the dialog content. Please go ahead with the needed changes.

I added missed javadoc comments and two new titles for "Server Unavailable" and "Connection restored" dialogs, changed the most dialogs to be cancelable on X button.
Please review changes rev 16337 (10915a). Thank you for formatting changes but I also had this in mind.

Hynek, I tested changes rev 16338 (10915a). The changes have no regressions that I just noted that the onCancel application trigger executes the same action to hide dialog as a the default trigger.

OK, I will check this later because working on the different task now. Although I tested direct upload and the old approach and it could be that missed these issues.

revno: 16349
branch nick: 10915a
message:
missed tests after changes in the dialog close listener logic, fixed the review notes (refs: #10915-54)

Sergey Ivanovskiy wrote:

Hynek, please review, the mouse handler was not restored because I missed tests after the hot changes in onClose dialog listener. I added debug logs for you. This test test-web-file-upload-1.p looks fine.

I didn't test, but it looks good.

Hynek Cihlar wrote:

Sergey Ivanovskiy wrote:

Hynek, please review, the mouse handler was not restored because I missed tests after the hot changes in onClose dialog listener. I added debug logs for you. This test test-web-file-upload-1.p looks fine.

I didn't test, but it looks good.

Committed new rev 16353 (10915a). I tested direct case and the case that supports the additional dialog.

Sergey, if you haven't also please test the various edge cases. An upload is interrupted by the user, a connection error during upload, file selection dialog opened, but no files selected, etc.

Hynek Cihlar wrote:

In dojo.connect(u, "onComplete", function(files) sendFileChoose is called conditionally if files evaluates to true. But sendFileChoose should be called unconditionally because server waits for the MSG_FILE_CHOOSE response.

Sergey, please also respond to this concern.

Hynek Cihlar wrote:

Hynek Cihlar wrote:

In dojo.connect(u, "onComplete", function(files) sendFileChoose is called conditionally if files evaluates to true. But sendFileChoose should be called unconditionally because server waits for the MSG_FILE_CHOOSE response.

Sergey, please also respond to this concern.

Hynek, this is discussed in my response to your message #10915-56

Hynek Cihlar wrote:

Sergey, if you haven't also please test the various edge cases. An upload is interrupted by the user, a connection error during upload, file selection dialog opened, but no files selected, etc.

Yes.

Sergey Ivanovskiy wrote:

Hynek Cihlar wrote:

Hynek Cihlar wrote:

In dojo.connect(u, "onComplete", function(files) sendFileChoose is called conditionally if files evaluates to true. But sendFileChoose should be called unconditionally because server waits for the MSG_FILE_CHOOSE response.

Sergey, please also respond to this concern.

Hynek, this is discussed in my response to your message #10915-56

#10915-56 only addresses the restore of me.processMouse. Or am I missing something?

Yes, I think that you missed. I tested the corresponding testcase and found that sendFileChoose is invoked if files are selected. The files are selected that means files is not null. Correct?

Is there a possible case to get onComplete message with files object evaluated to false? I think that it is not possible.

I changed my opinion and committed the corresponding changes as rev 16354 (10915a) where this case was taken into account.

Sergey Ivanovskiy wrote:

Is there a possible case to get onComplete message with files object evaluated to false? I think that it is not possible.

In that case the added if (statement) doesn't make sense and only renders the code hard to follow. Its presence implies files could evaluate to false.

Hynek Cihlar wrote:

Sergey Ivanovskiy wrote:

Is there a possible case to get onComplete message with files object evaluated to false? I think that it is not possible.

In that case the added if (statement) doesn't make sense and only renders the code hard to follow. Its presence implies files could evaluate to false.

I agree that makes sense for me. What additional tests should be done for merging this task?

Sergey Ivanovskiy wrote:

I agree that makes sense for me. What additional tests should be done for merging this task?

Ideally all the modal dialogs affected and the related UI flows should be tested.

The changes in 10915a look good.

10915a was rebased up to rev 16383.

Could this branch be merged into the trunk?

I rebased up to rev 16426 (10915a) and tested with the large customer application. Found the translation issue, the translation messages with spaces between words were braced with escaped double quotes, and finally the escaped double quotes \" were added around them and prevented keys to be translated.
Fixed this issue with rev 16427 (10915a), please review.

Committed revision 16428 (10915a) fixed the found configuration issue, the progress bar dialog was changed and closed button was added to this dialog because it should cache the close button dialog loaded by

<link rel="stylesheet" type="text/css" href="${webRoot}/dojo-toolkit/dijit/themes/claro/Dialog.css">

Hynek, could you review this task. I tested the changes, found issues and fixed them. Now the manual tests passed. Could we commit this task after the review?

Sergey Ivanovskiy wrote:

Hynek, could you review this task. I tested the changes, found issues and fixed them. Now the manual tests passed. Could we commit this task after the review?

Yes, sorry for the delay, I'm on it now.

Code review 10915a.

Just one point. I think removeEscapedDoubleQuotes will throw StringIndexOutOfBoundsException when the input is \".

Otherwise the changes look good. Any regression testing remaining?

Hynek Cihlar wrote:

Code review 10915a.

Just one point. I think removeEscapedDoubleQuotes will throw StringIndexOutOfBoundsException when the input is \".

Yes, what would you you propose? I can add a check.

Otherwise the changes look good. Any regression testing remaining?

It seems that I tested the all cases of dialogs.

Sergey Ivanovskiy wrote:

Hynek Cihlar wrote:

Code review 10915a.

Just one point. I think removeEscapedDoubleQuotes will throw StringIndexOutOfBoundsException when the input is \".

Yes, what would you you propose? I can add a check.

I would just remove the quote and return the rest of the string. Unless you had a different intention with the method.

The aim was to remove only paired escaped double quotes. Committed revision 16433 (10915a). Rebasing the branch.

Hynek Cihlar wrote:

Code review 10915a.

Just one point. I think removeEscapedDoubleQuotes will throw StringIndexOutOfBoundsException when the input is \".

Otherwise the changes look good. Any regression testing remaining?

Committed rev 16436 (10915a).

Why is this in Feedback status? (Feedback is reserved for waiting for a customer response).

Rebased revision 16490 (10915a).

Hynek: Please review.

Greg Shah wrote:

Hynek: Please review.

Actually the code was already reviewed. The problem can be related to missed issues while testing these changes.

I tested 10915a and found two issues: one is related to progress bar cancel button and the other issue is related to the partly broken reconnect logic that was incorrectly tested previously. All of them are fixed.

Could this branch be reviewed and be merged into the trunk? The last commits fixed found issues that was reproduced when the network is switched or failed and restored. Thus it contains also the fix for the trunk.
Although I can extract this fix into a separate task where it was introduced.

Rebased up to 16528 (10915a). This revision needs a final review.

Hynek Cihlar wrote:

• [MAJOR] functional p2j.js.close: After destroyRecursive(false) is called on that.dojoDialog, the next statement accesses that.dojoDialog.domNode and attempts that.container.removeChild(...). If dijit's destroy() nulls domNode (as specified in _WidgetBase), the removeChild is unreachable dead code. If it does not (version-dependent behavior), removeChild throws NotFoundError because the node was already removed from the DOM by destroyRecursive. In the exception case, all subsequent cleanup is skipped - that.disposed is never set to true, dialogs.delete(that.id) never executes, and listener/trigger cleanup is bypassed, leaving a stale entry in the dialogs Set that causes isDialogDisplayed() to permanently return true, blocking keyboard input. Additionally, that.unregistry() called further down attempts destroyRecursive a second time on the already-destroyed widget (guarded by try/catch, but redundant). Suggested fix: remove the removeChild block (lines 2780-2783) and the redundant unregistry() call (line 2811), since destroyRecursive(false) already handles DOM removal and registry cleanup.

I did not observe errors and this part is generated, is not?

It looks like no sense.

Taking into account that this part was manually written.

I think that such type of review is good but the sequence of this logic should have a proof even if the large LLM can bit man in many tasks. The offline LLM that I used recently produced incorrect code in a simple task and the generated code can be used only after several prompts to fix compiler errors.

Sergey, the review is generated by AI and currated by myself. I always check the AI outputs. I looked in the code and the points make sense to me. Of course I may be wrong.

Hynek Cihlar wrote:

Sergey, the review is generated by AI and currated by myself. I always check the AI outputs. I looked in the code and the points make sense to me. Of course I may be wrong.

This is not a problem. The issue here is that this part was written intentionally. I think this part is difficult because it relates to dijit widgets and must be proven. The proof can be the code that works incorrectly.

Sergey Ivanovskiy wrote:

Hynek Cihlar wrote:

Sergey, the review is generated by AI and currated by myself. I always check the AI outputs. I looked in the code and the points make sense to me. Of course I may be wrong.

This is not a problem. The issue here is that this part was written intentionally. I think this part is difficult because it relates to dijit widgets and must be proven. The proof can be the code that works incorrectly.

Which point do you mean?

Hynek, all sequence of generated reasoning can be look like truth. You should give a proof.

Hynek Cihlar wrote:

• [MAJOR] functional p2j.js.close: After destroyRecursive(false) is called on that.dojoDialog, the next statement accesses that.dojoDialog.domNode and attempts that.container.removeChild(...). If dijit's destroy() nulls domNode (as specified in _WidgetBase), the removeChild is unreachable dead code. If it does not (version-dependent behavior), removeChild throws NotFoundError because the node was already removed from the DOM by destroyRecursive. In the exception case, all subsequent cleanup is skipped - that.disposed is never set to true, dialogs.delete(that.id) never executes, and listener/trigger cleanup is bypassed, leaving a stale entry in the dialogs Set that causes isDialogDisplayed() to permanently return true, blocking keyboard input. Additionally, that.unregistry() called further down attempts destroyRecursive a second time on the already-destroyed widget (guarded by try/catch, but redundant). Suggested fix: remove the removeChild block (lines 2780-2783) and the redundant unregistry() call (line 2811), since destroyRecursive(false) already handles DOM removal and registry cleanup.

This review describes a error in the code ( error scenario) but actually the code has no logged errors.

Sergey Ivanovskiy wrote:

Hynek, all sequence of generated reasoning can be look like truth. You should give a proof.

Sergey, I typically don't test all the cases when reviewing code. This would be an overkill. I often point out potential issues, and yes I may be wrong. The point is we both settle on an agreement whether the points are real issues or not. In this case you have much better knowledge of the code changes and the affected use cases, so you may recognize the pointed issue is a non-issue right away, in which case I'm fine with that. But you could also realize this was a gap in the implementation.

Sergey Ivanovskiy wrote:

Hynek Cihlar wrote:

• [MAJOR] functional p2j.js.close: After destroyRecursive(false) is called on that.dojoDialog, the next statement accesses that.dojoDialog.domNode and attempts that.container.removeChild(...). If dijit's destroy() nulls domNode (as specified in _WidgetBase), the removeChild is unreachable dead code. If it does not (version-dependent behavior), removeChild throws NotFoundError because the node was already removed from the DOM by destroyRecursive. In the exception case, all subsequent cleanup is skipped - that.disposed is never set to true, dialogs.delete(that.id) never executes, and listener/trigger cleanup is bypassed, leaving a stale entry in the dialogs Set that causes isDialogDisplayed() to permanently return true, blocking keyboard input. Additionally, that.unregistry() called further down attempts destroyRecursive a second time on the already-destroyed widget (guarded by try/catch, but redundant). Suggested fix: remove the removeChild block (lines 2780-2783) and the redundant unregistry() call (line 2811), since destroyRecursive(false) already handles DOM removal and registry cleanup.

This review describes a error in the code ( error scenario) but actually the code has no logged errors.

I don't think it says you should see an error logged. "If dijit's destroy() nulls domNode (as specified in _WidgetBase), the removeChild is unreachable dead code. If it does not version-dependent behavior)...".

But If you are sure this is fine and don't see any bad behavior, let's just dismiss this as a non-issue.

Hynek Cihlar wrote:

Sergey Ivanovskiy wrote:

Hynek Cihlar wrote:

• [MAJOR] functional p2j.js.close: After destroyRecursive(false) is called on that.dojoDialog, the next statement accesses that.dojoDialog.domNode and attempts that.container.removeChild(...). If dijit's destroy() nulls domNode (as specified in _WidgetBase), the removeChild is unreachable dead code. If it does not (version-dependent behavior), removeChild throws NotFoundError because the node was already removed from the DOM by destroyRecursive. In the exception case, all subsequent cleanup is skipped - that.disposed is never set to true, dialogs.delete(that.id) never executes, and listener/trigger cleanup is bypassed, leaving a stale entry in the dialogs Set that causes isDialogDisplayed() to permanently return true, blocking keyboard input. Additionally, that.unregistry() called further down attempts destroyRecursive a second time on the already-destroyed widget (guarded by try/catch, but redundant). Suggested fix: remove the removeChild block (lines 2780-2783) and the redundant unregistry() call (line 2811), since destroyRecursive(false) already handles DOM removal and registry cleanup.

This review describes a error in the code ( error scenario) but actually the code has no logged errors.

I don't think it says you should see an error logged. "If dijit's destroy() nulls domNode (as specified in _WidgetBase), the removeChild is unreachable dead code. If it does not version-dependent behavior)...".

But If you are sure this is fine and don't see any bad behavior, let's just dismiss this as a non-issue.

I think so because this code was written intentionally after manual tests but taking into account the review there should be proofs that this is a real issue or not. Thus I need to check the code by tests. The problem here is that there are no testcases that cover this code.

I think that this methodology to apply AI helpers to review should be together with tests coverage.

Sergey Ivanovskiy wrote:

I think that this methodology to apply AI helpers to review should be together with tests coverage.

That would be ideal, but not that easy to do.

Sergey Ivanovskiy wrote:

Hynek Cihlar wrote:

Sergey Ivanovskiy wrote:

Hynek Cihlar wrote:

• [MAJOR] functional p2j.js.close: After destroyRecursive(false) is called on that.dojoDialog, the next statement accesses that.dojoDialog.domNode and attempts that.container.removeChild(...). If dijit's destroy() nulls domNode (as specified in _WidgetBase), the removeChild is unreachable dead code. If it does not (version-dependent behavior), removeChild throws NotFoundError because the node was already removed from the DOM by destroyRecursive. In the exception case, all subsequent cleanup is skipped - that.disposed is never set to true, dialogs.delete(that.id) never executes, and listener/trigger cleanup is bypassed, leaving a stale entry in the dialogs Set that causes isDialogDisplayed() to permanently return true, blocking keyboard input. Additionally, that.unregistry() called further down attempts destroyRecursive a second time on the already-destroyed widget (guarded by try/catch, but redundant). Suggested fix: remove the removeChild block (lines 2780-2783) and the redundant unregistry() call (line 2811), since destroyRecursive(false) already handles DOM removal and registry cleanup.

This review describes a error in the code ( error scenario) but actually the code has no logged errors.

I don't think it says you should see an error logged. "If dijit's destroy() nulls domNode (as specified in _WidgetBase), the removeChild is unreachable dead code. If it does not version-dependent behavior)...".

But If you are sure this is fine and don't see any bad behavior, let's just dismiss this as a non-issue.

I think so because this code was written intentionally after manual tests but taking into account the review there should be proofs that this is a real issue or not. Thus I need to check the code by tests. The problem here is that there are no testcases that cover this code.

I think AI is correct here and this is also why I kept it as an issue. If you look in your code:

             if (that.dojoDialog)
               {
                  //do not preserve dom
                  that.dojoDialog.destroyRecursive(false);
               }
               if (that.dojoDialog.domNode)
               {
                  that.container.removeChild(that.dojoDialog.domNode);
               }

In the call that.dojoDialog.destroyRecursive(false) the supplied argument tells dojo it should not preserve the dom and so

               if (that.dojoDialog.domNode)
               {
                  that.container.removeChild(that.dojoDialog.domNode);
               }

either adds noise or could even cause issues (even in the future when dojo version changes) if domNode is not nulled. Exactly as AI identified.

Hynek Cihlar wrote:

Sergey Ivanovskiy wrote:

Hynek Cihlar wrote:

Sergey Ivanovskiy wrote:

Hynek Cihlar wrote:

• [MAJOR] functional p2j.js.close: After destroyRecursive(false) is called on that.dojoDialog, the next statement accesses that.dojoDialog.domNode and attempts that.container.removeChild(...). If dijit's destroy() nulls domNode (as specified in _WidgetBase), the removeChild is unreachable dead code. If it does not (version-dependent behavior), removeChild throws NotFoundError because the node was already removed from the DOM by destroyRecursive. In the exception case, all subsequent cleanup is skipped - that.disposed is never set to true, dialogs.delete(that.id) never executes, and listener/trigger cleanup is bypassed, leaving a stale entry in the dialogs Set that causes isDialogDisplayed() to permanently return true, blocking keyboard input. Additionally, that.unregistry() called further down attempts destroyRecursive a second time on the already-destroyed widget (guarded by try/catch, but redundant). Suggested fix: remove the removeChild block (lines 2780-2783) and the redundant unregistry() call (line 2811), since destroyRecursive(false) already handles DOM removal and registry cleanup.

This review describes a error in the code ( error scenario) but actually the code has no logged errors.

I don't think it says you should see an error logged. "If dijit's destroy() nulls domNode (as specified in _WidgetBase), the removeChild is unreachable dead code. If it does not version-dependent behavior)...".

But If you are sure this is fine and don't see any bad behavior, let's just dismiss this as a non-issue.

I think so because this code was written intentionally after manual tests but taking into account the review there should be proofs that this is a real issue or not. Thus I need to check the code by tests. The problem here is that there are no testcases that cover this code.

I think AI is correct here and this is also why I kept it as an issue. If you look in your code:

[...]

In the call that.dojoDialog.destroyRecursive(false) the supplied argument tells dojo it should not preserve the dom and so
[...]

either adds noise or could even cause issues (even in the future when dojo version changes) if domNode is not nulled. Exactly as AI identified.

This is not what this review state. unregistry and destroyRecursive is not the same and if unregistry is not invoked, then the next call to createDialog would fail.
I will check this part

               if (that.dojoDialog)
               {
                  //do not preserve dom
                  that.dojoDialog.destroyRecursive(false);
               }
               if (that.dojoDialog.domNode)
               {
                  that.container.removeChild(that.dojoDialog.domNode);
               }

Hynek Cihlar wrote:

• [MINOR] functional p2j.js.createDojoDialog (onCancel handler): When the user clicks the X close button, dijit fires onCancel. The handler calls the application onCancel listener (which for the reconnect/unavailable dialogs calls close()), and then unconditionally calls that.hide(). Since close() is asynchronous (deferred-based), hide() runs before the close chain completes, resulting in both close() and hide() async chains operating on the same dialog. While unlikely to crash (dijit's hide() is mostly idempotent), this is fragile. Consider guarding the hide() call with if (!that.disposed) or removing it entirely when the listener already calls close().

Dialog.hide() returns a promise but it also set this._set("open", false); and according this code

        hide: function(){
            // summary:
            //        Hide the dialog
            // returns: dojo/promise/Promise
            //        Promise object that resolves when the display animation is complete

            // If we haven't been initialized yet then we aren't showing and we can just return.
            // Likewise if we are already hidden, or are currently fading out.
            if(!this._alreadyInitialized || !this.open){
                return resolvedDeferred.promise;
            }

Finally, the major issue is actually the code that is not performed

               if (that.dojoDialog.domNode)
               {
                  that.container.removeChild(that.dojoDialog.domNode);
               }

Sergey Ivanovskiy wrote:

This is not what this review state. unregistry and destroyRecursive is not the same and if unregistry is not invoked, then the next call to createDialog would fail.
I will check this part
[...]
but it is not what the previous text states.

Actually it is. I was able to locate the code without any problem. The mention of unregistry is a second and related issue. AI correctly states that destroyRecursive is called for the second time. Frankly, not a big problem, just unnecessary noise that can be easily resolved I believe.

Hynek, we don't have a way to debug dojo jscript code from the box, do we? DojoToolkitHandler currently loads requested resources from dojo-1.17.3-distribution.zip that contains uncompressed js code too. Could we implement the debug option to load uncompressed versions *.js.uncompressed.js of target resources *.js in order to debug code easily?

Gemini noted that there two versions of dojo Legacy Dojo (1.x) and Modern Dojo (v2+). It seems that we are using Legacy Dojo (1.x).

Found that the issue was related to FileList widget that is not finished asynchronous operation while the result notification dialog has been already closed so all its content has been disposed. I have a fix and can add debug opition to enable debugging Legacy Dojo (1.x).
I don't know if it makes sense to consider Modern Dojo (v2+) for FWD web client.

Sergey Ivanovskiy wrote:

I don't know if it makes sense to consider Modern Dojo (v2+) for FWD web client.

What does Dojo 2 bring for this issue?

Modern Dojo (v2+) https://dojo.io/ is a complete reboot of the framework, not compatible with the old versions Legacy Dojo (1.x).

Sergey Ivanovskiy wrote:

Modern Dojo (v2+) https://dojo.io/ is a complete reboot of the framework, not compatible with the old versions Legacy Dojo (1.x).

I mean does it solve any pains we have? Do we need it? Besides, version 2 is still in beta.

Hynek Cihlar wrote:

Sergey Ivanovskiy wrote:

Modern Dojo (v2+) https://dojo.io/ is a complete reboot of the framework, not compatible with the old versions Legacy Dojo (1.x).

I mean does it solve any pains we have? Do we need it? Besides, version 2 is still in beta.

Legacy Dojo (1.x). looks good, fitted for this task.

[MAJOR] functional p2j.socket.js.checkLostPings: The ping-pong watcher timeout is halved via pingPongInterval >> 1 and maxLostPings changed from Math.trunc(maxIdleTime / pingPongInterval) + 1 to Math.max(Math.trunc(maxIdleTime / (2 * pingPongInterval)), 2). The effective detection window is now roughly maxIdleTime/4 instead of maxIdleTime, making the client detect lost connections approximately 4x faster. Was this intentional?

Yes, the issue was that pingPong listener was canceled before the lost ping was detected, finally watchdog timeout was hit. I missed this issue because tested incorrectly. The correct test should be switching network or turn off the Ethernet cable but my test was only to kill the server.

[MAJOR] functional p2j.js.createOpenPopupDialog: The guard changed from !openPopupDialog || openPopupDialog.disposed to !isDialogDisplayed(), which checks whether any ModalDialog is in the dialogs Set (via dialogs.size > 0), not specifically the open-popup dialog. If an unrelated dialog is displayed, createOpenPopupDialog will silently skip creation. On first call with another dialog open, openPopupDialog is undefined, and the else branch calling openPopupDialog.addTrigger(...) would throw a TypeError. This is a functional regression from the original instance-specific check.

Yes, it is an issue. The changes was done mechanically.

[MAJOR] functional p2j.socket.js: idleTimer changed from maxIdleTime to Math.max(watchdogTimeout, maxIdleTime) and connectivityTimer changed from Math.min(watchdogTimeout, maxIdleTime) to Math.max(watchdogTimeout, maxIdleTime). The client now waits longer before giving up on reconnection attempts. This is related to the previous point, was it intentional?

Yes, tried to wait longer.

[MAJOR] functional p2j.js.onCancel: The onCancel handler on the dojo dialog calls that.hide() unconditionally after invoking the application listener. But restoreConnectionDialogEscapeTrigger and serverUnavailableDialogEscapeTrigger (registered via addTrigger("onCancel", ...)) call close(), which internally calls dojoDialog.hide(). Since close() is async but not awaited by the synchronous listener() call, the onCancel handler immediately proceeds to call that.hide(). Both close() and hide() then race to call that.dojoDialog.hide() on the underlying dojo widget, and close() may null dojoDialog before hide() uses it.

Thank you for finding. I tried to fix all similar issues.

[MAJOR] functional p2j.js.onCancel: The onCancel handler on the dojo dialog calls that.hide() unconditionally after invoking the application listener. But restoreConnectionDialogEscapeTrigger and serverUnavailableDialogEscapeTrigger (registered via addTrigger("onCancel", ...)) call close(), which internally calls dojoDialog.hide(). Since close() is async but not awaited by the synchronous listener() call, the onCancel handler immediately proceeds to call that.hide(). Both close() and hide() then race to call that.dojoDialog.hide() on the underlying dojo widget, and close() may null dojoDialog before hide() uses it.

[MAJOR] functional p2j.js.show / hide: Both are async functions that await createDialogAction. If show() is called and then hide() is called before show() completes its await, hide() checks isVisible() which is still false (not yet set by show) and returns early. Then show() resumes, sets visible=true, adds to dialogs Set. The dialog remains visible with no way to dismiss it. There is no re-check of desired state after the await.

These two should be resolved using the chain of promises, actually the resolved promise that represents the last UI operation and the next UI operation starts only after the last operation is done.

Rebased rev. 16564 (10915a).

Code review 10915a revisions 16537..16564

• [CRITICAL] functional ModalDialog.show: Returns this.operationGuard instead of the local operationGuard variable. operationGuard is declared as a local var and is never assigned as a property on this, so await dialog.show() resolves to undefined. Every other async API method (hide, close, setTitle, setContent, setTextContent, setButtons, setFooter, overrideDialogStyle, overrideTitleStyle, updateLayout) correctly returns the local operationGuard. Callers awaiting show() lose the ordering guarantee the operationGuard pattern is meant to provide. This is the critical bug flagged in the Redmine summary as not yet confirmed fixed - it remains in the diff.

• [MAJOR] functional ModalDialog.doDialogLayout: Interleaves DOM writes and reads in a way that forces layout thrashing on every show. Writes topPane.domNode.style.height, footerPane.domNode.style.height, dojoDialog.domNode.style.height; then reads offsetHeight on multiple panes; then writes style.top values; then reads four pairs of scrollWidth/offsetWidth; then writes style.width. Each read after a style write triggers a forced synchronous reflow. Repeated unconditionally on every show() and updateLayout(). Group reads first then writes, or schedule via requestAnimationFrame.

• [MAJOR] functional ModalDialog.show: After awaiting dojoDialog.show(), the code calls that.dojoDialog.containerNode.focus() unconditionally. This overrides the autofocus: true setting that already focused the first focusable child (e.g. an OK button created by setButtons). Breaks "Press Enter to confirm" UX because focus is moved off the default button.

• [MAJOR] functional onKeyDown: Moving the p2j.isDialogDisplayed() early-return to the start of keydown skips the OS modifier-key propagation p2j.socket.sendOSKeyEvent(keyCode, false) that previously executed before the dialog gate. While dialogs are displayed, the server no longer receives Alt/Ctrl/Shift up/down notifications, leaving the server's modifier state out of sync once the dialog closes. The pre-processing of keyPressEmitted = false; lastPressedKey = undefined; and activateKeyboardInput(forceActivateInput) is also skipped. Now I'm not sure this is an actual issie, but worth to check.

• [MAJOR] functional chooseFiles: The new dojo.connect(u, "onProgress", ...) handler unconditionally calls actionResultsDialog.show(). onProgress fires for every chunk during upload, and each show() enqueues an operationGuard promise step that runs doDialogLayout() when the dialog is already visible (forced reads of scrollHeight/offsetHeight followed by style writes - synchronous layout per tick). The previous code triggered no show() in onProgress and a guarded show() in onError; both guards are now gone. Add an if (!actionResultsDialog.isVisible()) guard, or drop the show() from onProgress since onChange already shows the dialog before upload starts.

• [MAJOR] style Utils: History entry 111 SBI 20260206 Added removeDoubleQuotesArround. has typo "Arround" (should be "Around") and references the wrong method name (removeDoubleQuotesArround vs actual removeEscapedDoubleQuotes). The history description must match the symbol name.

• [MAJOR] style p2j.keyboard.js history entry 040: Typo "dyalog" should be "dialog" in the description ("Rejected processing keydown event if the web client modal dyalog is opened.").

• [MAJOR] style chooseFiles: Mis-indented if (!multiple) block (8 spaces leading whitespace instead of 9), making it one column shy of its surrounding block - the matching brace below uses 9 spaces.

• [MAJOR] style ModalDialog.createDojoDialog: Mixed indentation depths inside unregistry() - some inner try { ... } blocks use 2-space indentation instead of the project's 3-space step. Inside the new Dialog({...}) literal, key spacing is inconsistent: id : that.id next to className: "dialogFWDOverride" and autofocus: true.

• [MAJOR] style ModalDialog.doDialogLayout: Lines exceed the 110-character limit, e.g. var borderPanelWidth = Math.max(borderContainer.domNode.scrollWidth,borderContainer.domNode.offsetWidth); and the immediately following borderPanelWidth = Math.max(...) line. Also missing space after the comma in Math.max(...,borderContainer...).

• [MINOR] functional p2j: Duplicate var dialogs = new Set(); declaration introduced in p2j.js. The original line was kept and an identical second declaration with the same JSDoc comment was added immediately below it. Functionally harmless because both Sets are empty at this point with no intervening writes (the second simply overwrites the first reference before any element is added), but a clear copy-paste/merge artifact that should be removed.

• [MINOR] functional DojoToolkitHandler.mapToResource: When debugMode is on, every *.js request (not already *.js.uncompressed.js) is rewritten to *.js.uncompressed.js with no fallback; AbstractModulesHandler.deliverModule silently returns HTTP 404 if the resource is missing. The dojo-1.17.3 distribution contains 14 plain .js files without an .uncompressed.js sibling (e.g. configRhino.js, configNode.js, profile/build artifacts) - mostly non-browser build/config files unlikely to be requested in normal runtime, but the rewrite is still brittle. Either fall back to the original target on 404, or scope the rewrite to known modules.

• [MINOR] functional ModalDialog.show: When show() is called while isVisible() is already true, the new code calls that.doDialogLayout() unconditionally - so even no-op shows force a full layout pass (DOM reads/writes plus borderContainer.resize()). The old direct-DOM implementation did nothing in this case.

• [MINOR] functional ModalDialog: this.removeTrigger = removeTrigger; was removed but this.addTrigger remains. The removeTrigger function is still defined and used internally (in close()'s forEach loops) but is no longer exposed on the dialog instance. External callers can no longer remove a previously-added trigger - backward-incompatible API surface reduction.

• [MINOR] performance ModalDialog.createDojoDialog: dojo.require(...) and AMD require([...10 modules...]) run on every dialog construction. While modules are cached, each call still walks the dependency list and queues a microtask. For dialogs created repeatedly (loadingDialog, connection dialogs, file-upload progress), hoisting the require once and caching resolved modules would amortize the cost. In DOJO_SOURCE_DEBUG mode the impact is larger because cacheBust=true defeats browser cache.

• [MINOR] performance p2j.dialogs Set: Now stores dialog ID strings rather than DOM nodes. If two ModalDialog instances are created with the same id (e.g. recreating after close()), dialogs.delete(that.id) from the first instance silently removes the tracker for the second. The loadingDialog/restoreConnectionDialog/connectionRestoredDialog/serverUnavailableDialog IDs are reused across reconnects. Please check whether this is an actual issue.

• [MINOR] style ModalDialog.logError: JsDoc @return {Function}@ description is empty. Either remove the tag or document what is returned.

• [MINOR] style p2j.client.css: Mixed indentation in new rules (some lines 11 spaces, some 23 spaces) instead of consistent 3-space steps. Trailing whitespace after } on the .dijitDialogPaneContent.dijitDialogSingleChild block.

• [MINOR] style p2j.socket.js.checkLostPings: Indentation broken - the opening if (ws) is at 9 spaces but inner blocks use 12 spaces, while the comment lines start at column 8, creating misaligned visual nesting.

Hynek Cihlar wrote:

Code review 10915a revisions 16537..16564

• [CRITICAL] functional ModalDialog.show: Returns this.operationGuard instead of the local operationGuard variable. operationGuard is declared as a local var and is never assigned as a property on this, so await dialog.show() resolves to undefined. Every other async API method (hide, close, setTitle, setContent, setTextContent, setButtons, setFooter, overrideDialogStyle, overrideTitleStyle, updateLayout) correctly returns the local operationGuard. Callers awaiting show() lose the ordering guarantee the operationGuard pattern is meant to provide. This is the critical bug flagged in the Redmine summary as not yet confirmed fixed - it remains in the diff.

Hynek, what does this note "This is the critical bug flagged in the Redmine summary as not yet confirmed fixed - it remains in the diff." mean?

[MAJOR] functional onKeyDown: Moving the p2j.isDialogDisplayed() early-return to the start of keydown skips the OS modifier-key propagation p2j.socket.sendOSKeyEvent(keyCode, false) that previously executed before the dialog gate. While dialogs are displayed, the server no longer receives Alt/Ctrl/Shift up/down notifications, leaving the server's modifier state out of sync once the dialog closes. The pre-processing of keyPressEmitted = false; lastPressedKey = undefined; and activateKeyboardInput(forceActivateInput) is also skipped. Now I'm not sure this is an actual issue, but worth to check.

The idea in this code is to prevent sending keyboard events to the business logic because the implemented dialogs are not involved into the business logic.

[MAJOR] functional chooseFiles: The new dojo.connect(u, "onProgress", ...) handler unconditionally calls actionResultsDialog.show(). onProgress fires for every chunk during upload, and each show() enqueues an operationGuard promise step that runs doDialogLayout() when the dialog is already visible (forced reads of s@crollHeight/offsetHeight@ followed by style writes - synchronous layout per tick). The previous code triggered no show() in onProgress and a guarded show() in onError; both guards are now gone. Add an if (!actionResultsDialog.isVisible()) guard, or drop the show() from onProgress since onChange already shows the dialog before upload starts.

The idea in this code is to adjust dialog size but it seems onChange should update the dialog size according to the selected files.

[MINOR] functional ModalDialog.show: When show() is called while isVisible() is already true, the new code calls that.doDialogLayout() unconditionally - so even no-op shows force a full layout pass (DOM reads/writes plus borderContainer.resize()). The old direct-DOM implementation did nothing in this case.

Yes, if the dialog is already visible, then the code calls doDialogLayout().

[MAJOR] functional ModalDialog.doDialogLayout: Interleaves DOM writes and reads in a way that forces layout thrashing on every show. Writes topPane.domNode.style.height, footerPane.domNode.style.height, dojoDialog.domNode.style.height; then reads offsetHeight on multiple panes; then writes style.top values; then reads four pairs of scrollWidth/offsetWidth; then writes style.width. Each read after a style write triggers a forced synchronous reflow. Repeated unconditionally on every show() and updateLayout(). Group reads first then writes, or schedule via requestAnimationFrame.

This is the code. Could you explain if this is possible to improve here something?

               function doDialogLayout()
               {
                  var dim = computeDialogSize();
//                  console.debug("doDialogLayout(): dim=" + JSON.stringify(dim));
                  topPane.domNode.style.height = dim.topHeight + 'px';
                  footerPane.domNode.style.height = dim.footerHeight + 'px';
                  dojoDialog.domNode.style.height = dim.height + 'px';

                  var topPanelHeight = topPane.domNode.offsetHeight;
                  var centerPanelHeight = centerPane.domNode.offsetHeight;
                  centerPane.domNode.style.top = topPane.domNode.offsetHeight + 'px';
                  footerPane.domNode.style.top = (topPane.domNode.offsetHeight + centerPane.domNode.offsetHeight) + 'px';

                  var centerPanelWidth = Math.max(centerPane.domNode.scrollWidth, centerPane.domNode.offsetWidth);
                  var topPanelWidth = Math.max(topPane.domNode.scrollWidth, topPane.domNode.offsetWidth);
                  var footerPanelWidth = Math.max(footerPane.domNode.scrollWidth, footerPane.domNode.offsetWidth);
                  var borderPanelWidth = Math.max(borderContainer.domNode.scrollWidth,borderContainer.domNode.offsetWidth);
                  borderPanelWidth = Math.max(dim.width, topPanelWidth, centerPanelWidth, footerPanelWidth, borderPanelWidth);

                  borderContainer.domNode.style.width = borderPanelWidth + 'px';
                  borderContainer.resize();
               }
               that.doDialogLayout = doDialogLayout;

Sergey Ivanovskiy wrote:

[MAJOR] functional onKeyDown: Moving the p2j.isDialogDisplayed() early-return to the start of keydown skips the OS modifier-key propagation p2j.socket.sendOSKeyEvent(keyCode, false) that previously executed before the dialog gate. While dialogs are displayed, the server no longer receives Alt/Ctrl/Shift up/down notifications, leaving the server's modifier state out of sync once the dialog closes. The pre-processing of keyPressEmitted = false; lastPressedKey = undefined; and activateKeyboardInput(forceActivateInput) is also skipped. Now I'm not sure this is an actual issue, but worth to check.

The idea in this code is to prevent sending keyboard events to the business logic because the implemented dialogs are not involved into the business logic.

Fair enough.

Sergey Ivanovskiy wrote:

Hynek, what does this note "This is the critical bug flagged in the Redmine summary as not yet confirmed fixed - it remains in the diff." mean?

This means the issue was already identified in one of the previous reviews.

Sergey Ivanovskiy wrote:

[MAJOR] functional ModalDialog.doDialogLayout: Interleaves DOM writes and reads in a way that forces layout thrashing on every show. Writes topPane.domNode.style.height, footerPane.domNode.style.height, dojoDialog.domNode.style.height; then reads offsetHeight on multiple panes; then writes style.top values; then reads four pairs of scrollWidth/offsetWidth; then writes style.width. Each read after a style write triggers a forced synchronous reflow. Repeated unconditionally on every show() and updateLayout(). Group reads first then writes, or schedule via requestAnimationFrame.

This is the code. Could you explain if this is possible to improve here something?

The idea is to group together the writes and reads. Perhaps something like the following.

function doDialogLayout()
{
   var dim = computeDialogSize();

   // Phase 1: writes that change measured heights
   topPane.domNode.style.height    = dim.topHeight + 'px';
   footerPane.domNode.style.height = dim.footerHeight + 'px';
   dojoDialog.domNode.style.height = dim.height + 'px';

   // Phase 2: all reads (one forced reflow, not three)
   var topH         = topPane.domNode.offsetHeight;
   var centerH      = centerPane.domNode.offsetHeight;
   var topW         = Math.max(topPane.domNode.scrollWidth,         topPane.domNode.offsetWidth);
   var centerW      = Math.max(centerPane.domNode.scrollWidth,      centerPane.domNode.offsetWidth);
   var footerW      = Math.max(footerPane.domNode.scrollWidth,      footerPane.domNode.offsetWidth);
   var borderW      = Math.max(borderContainer.domNode.scrollWidth, borderContainer.domNode.offsetWidth);
   var finalW       = Math.max(dim.width, topW, centerW, footerW, borderW);

   // Phase 3: remaining writes
   centerPane.domNode.style.top         = topH + 'px';
   footerPane.domNode.style.top         = (topH + centerH) + 'px';
   borderContainer.domNode.style.width  = finalW + 'px';
   borderContainer.resize();
}

But unless there are some visible layout shifts I would ignore this.

[MINOR] functional ModalDialog: this.removeTrigger = removeTrigger; was removed but this.addTrigger remains. The removeTrigger function is still defined and used internally (in close()'s forEach loops) but is no longer exposed on the dialog instance. External callers can no longer remove a previously-added trigger - backward-incompatible API surface reduction.

Yes, removeTrigger is used internally on close() to simplify ModalDialog usages.

[MINOR] performance p2j.dialogs Set: Now stores dialog ID strings rather than DOM nodes. If two ModalDialog instances are created with the same id (e.g. recreating after close()), dialogs.delete(that.id) from the first instance silently removes the tracker for the second. The loadingDialog/restoreConnectionDialog/connectionRestoredDialog/serverUnavailableDialog IDs are reused across reconnects. Please check whether this is an actual issue.

It is supposed that each ModalDialog dialog has its own unique id. The same dialog can be recreated after close().

Committed revision 16570 (10915a) should fix the most issues except

[MINOR] performance ModalDialog.createDojoDialog: dojo.require(...) and AMD require([...10 modules...]) run on every dialog construction. While modules are cached, each call still walks the dependency list and queues a microtask. For dialogs created repeatedly (loadingDialog, connection dialogs, file-upload progress), hoisting the require once and caching resolved modules would amortize the cost. In DOJO_SOURCE_DEBUG mode the impact is larger because cacheBust=true defeats browser cache.

Sergey Ivanovskiy wrote:

[MINOR] functional ModalDialog: this.removeTrigger = removeTrigger; was removed but this.addTrigger remains. The removeTrigger function is still defined and used internally (in close()'s forEach loops) but is no longer exposed on the dialog instance. External callers can no longer remove a previously-added trigger - backward-incompatible API surface reduction.

Yes, removeTrigger is used internally on close() to simplify ModalDialog usages.

Right, in that case it should not be exposed.

Sergey Ivanovskiy wrote:

[MINOR] performance p2j.dialogs Set: Now stores dialog ID strings rather than DOM nodes. If two ModalDialog instances are created with the same id (e.g. recreating after close()), dialogs.delete(that.id) from the first instance silently removes the tracker for the second. The loadingDialog/restoreConnectionDialog/connectionRestoredDialog/serverUnavailableDialog IDs are reused across reconnects. Please check whether this is an actual issue.

It is supposed that each ModalDialog dialog has its own unique id. The same dialog can be recreated after close().

I looked at this closer and this should not be an issue. Please dismiss this.

Each time AI assistant based on Claude Code finds new issues. I rebased this branch 10915a up to rev 16587.

Committed revision 16588 (10915a) minor changes found by the gcd-code-style review.

Running fwd-code-review reported these issues:

FWD Code Review — Curated Findings¶

Summary¶

The diff modifies 14 files across the FWD web client subsystem, primarily refactoring the client-side ModalDialog from custom HTML/CSS to Dojo dijit.Dialog with async operation queuing, autofocus/refocus, and proper lifecycle management. A new DOJO_SOURCE_DEBUG config flag serves uncompressed Dojo sources. Keyboard event handling moves the isDialogDisplayed() check earlier to block server-bound events when a modal is active. CSS is rewritten with Dojo dialog overrides, and various JS files are updated for the new dialog API.

Findings¶

• [CRITICAL] functional p2j.keyboard.js onkeydown (ChUI): No isDialogDisplayed() check exists in the ChUI keyboard handler. Only the GUI handler was modified. In ChUI mode, all keyboard events bypass the modal dialog guard entirely.

• [CRITICAL] functional ModalDialog createDojoDialog: No timeout if require() callback never fires. If Dojo module loading fails silently, createDialogDeferred is never resolved or rejected. All subsequent operations hang forever on await createDialogAction, leaving the dialog in a permanently broken state.

• [MAJOR] functional p2j.keyboard.js onkeyup: OS key events are sent to the server before the isDialogDisplayed() check. Only onkeydown was fixed in this diff. The server receives unbalanced key events with no matching keydowns, potentially corrupting key state tracking.

• [MAJOR] functional ModalDialog constructor (loading dialog): Loading dialog created with closable: true, allowing users to dismiss the system loading progress dialog via the close button. The old custom modal had no close button.

• [MAJOR] functional ModalDialog close: The close() method does not set that.visible = false. After close() completes, isVisible() returns stale state.

• [MAJOR] functional ModalDialog close: Calls dojoDialog.hide() unconditionally while hide() guards with isVisible(). Can fire spurious Dojo onHide callbacks.

• [MAJOR] functional ModalDialog close: onClose application listener (which restores me.processMouse) is called inside try block after dojoDialog.hide(). If hide() throws, listener is never invoked, leaving me.processMouse permanently false.

• [MAJOR] functional ModalDialog close: dialogs.delete(that.id) runs before onClose listener fires. During listener execution, isDialogDisplayed() returns false so keyboard events are not blocked.

• [MAJOR] functional ModalDialog close: dojoDialog.hide() called redundantly before unregistry() -> destroyRecursive(false). If Dojo's internal onHide fires during redundant hide() and throws, it prevents destroyRecursive from running, leaking widget state.

• [MAJOR] functional ModalDialog onCancel: that.hide() is fire-and-forget (un-awaited). If application onCancel listener calls that.close(), ordering surprises can occur for code listening to both onCancel and onClose.

• [MAJOR] functional p2j.keyboard.js onkeypress: keyPressEmitted = true is set before the isDialogDisplayed() check. When dialog blocks the event, the flag remains true, corrupting keydown/keypress synchronization for the next real keypress.

• [MAJOR] functional p2j.mouse.js processWindowBorder: The only isDialogDisplayed() check in mouse.js is for the resize cursor. processMouseMove can bypass the guard when sendMouseMove is true.

• [MAJOR] performance computeDialogSize/doDialogLayout: Layout thrashing. Setting style.height = 'auto' (write) followed immediately by reads of scroll/offset dimensions forces 2-3 synchronous browser reflows per dialog show.

• [MAJOR] performance ModalDialog close: After destroyRecursive disposes the Dojo widget, operationGuard chain keeps the closed-over environment alive via that. Fields that.centerPane, that.footerPane, that.topPane are never nulled, so the destroyed Dojo widget subtree remains reachable through the closure.

• [MAJOR] security p2j.screen.js chooseFiles (onError): JSON.stringify(error) is injected via innerHTML without HTML sanitization. Attacker-controlled file names with embedded HTML can execute script on upload failure.

• [MAJOR] style AbstractModulesHandler: No history entry added despite significant modifications (new field, constructor, changed method signature, modified deliverModule).

• [MAJOR] style WebResourceHandler: No history entry added and copyright year not updated (still 2018-2025). File was modified in 2026.

• [MAJOR] style p2j.js ModalDialog/createDojoDialog: Line exceeds 110-character limit (123 chars).

• [MAJOR] style p2j.js ModalDialog/createDojoDialog: Inconsistent colon spacing in object literal (mixes id : that.id and className: "...").

• [MINOR] style ConfigItem.java DOJO_SOURCE_DEBUG: Continuation-line indentation uses 10 spaces, not a multiple of 3-space rule.

• [MINOR] style p2j.js ModalDialog/createDojoDialog: Inconsistent brace placement for object literals (some use same-line {, others brace on own line).

• [MINOR] style p2j.js computeDialogSize: Inconsistent colon spacing in return object literal.

• [MINOR] style p2j.socket.js: Pre-existing duplicate sequence ID 081.

• [MINOR] security WebPageHandler.java WebPageKeysProvider: Configuration values substituted into script string literal via raw string replacement with no HTML/JS escaping. A config value containing a single quote would break out of the JS string context.

• [MINOR] security p2j.socket.js serverUnavailableDialogEscapeTrigger: console.error(e) on cross-origin navigation failure leaks error details to console.

• [MINOR] performance ModalDialog createDojoDialog: Loads dijit/form/Button but never instantiates or uses it.

• [MINOR] performance ModalDialog close: contentDiv and titleDiv never set to null after unregistry(), keeping destroyed DOM subtrees alive via closures.

• [MINOR] performance ModalDialog createDojoDialog: dojo.require("dojo.Deferred") called synchronously every time, adding redundant lookups.

• [MINOR] performance ModalDialog setTitle: that.noTitle = title.length == 0 evaluated unnecessarily for truthy cases regardless of whether title changed.

• [MINOR] performance p2j.socket.js connectivityTimer/idleTimer: Math.max instead of Math.min extends the reconnection loop lifetime.

• [MINOR] performance ModalDialog overrideDialogStyle/overrideTitleStyle: Async serialization for simple domClass.toggle call means style override cannot take effect until the entire create chain completes.

• [MINOR] performance ModalDialog: changeDisplayStyle/changeVisibilityStyle dead code from old dialog implementation.

• [MINOR] functional ModalDialog createDojoDialog: dojo.require("dojo.Deferred") is unnecessary with AMD-style require().

• [MINOR] functional ModalDialog overrideDialogStyle/overrideTitleStyle: API changed from style-object to CSS-class-name. Old callers using inline style objects will silently fail.

• [MINOR] functional p2j.js setLoadingCheckpoint: Progress updates continue on hidden (but not disposed) dialog after user dismisses it via X button.

• [MINOR] functional Focus state divergence on dialog close: autofocus: true/refocus: true on Dojo Dialog manages browser-level focus, but FWD server-side FocusManager is not notified.

• [MINOR] functional p2j.screen.js activateWindow: Returns undefined instead of false when isDialogDisplayed() is true.

• [MINOR] functional p2j.screen.js chooseFiles close: actionResultsDialog.close() and dialog.close() are not awaited.

• [MINOR] functional ModalDialog setButtons: After close() sets buttonsDiv = null, calling setButtons again silently does nothing.

• [MINOR] functional p2j.keyboard.js onkeyup: Missing && evt.key != "Process" filter compared to equivalent check in onkeydown.

No, this is related to the dojo code.

var okButton = new dijit.form.Button(
         {
            label : okButtonLabel,
            onClick: clickOnInputFieldToSelectFiles
         });

Committed revision 16591.(10915a). Fixed critical and major issues found in the AI review (#10915a-155).

Committed revision 16592 (10915a) fixed most of minor issues and I run new code review to collect what was left or ignored.

Stefanel, could you help with the test case to check the reauth dialog style from p2j.sso_reauth.js?

The current rev 16593 has this report

FWD Code Review — Curated Findings¶

Summary¶

This diff spans 20 files covering a ModalDialog rewrite (raw HTML to Dojo dijit/Dialog), connection management improvements, keyboard/mouse event hardening, Dojo source debugging support, and CSS restructuring. The previously identified CRITICAL issue (p2j.sso_reauth.js overrideDialogStyle API break) has been fixed — the caller now correctly passes CSS class name "ssoReauthDialog" with matching CSS class defined in p2j.client.css.

Findings¶

• [MAJOR] style p2j.js: History entry 034 first date (20251220) predates entry 033 (20260326). History entries should be in chronological order.

• [MAJOR] style p2j.socket.js: History entry 083 first date (20251220) predates entry 082 (20260326). Chronology error.

• [MAJOR] style p2j.screen.js: History entry 102 first date (20251220) predates entry 101 (20260504). Chronology error.

• [MAJOR] style Utils.java: History entry 111 first date (20260206) predates entry 110 (20260402). Chronology error.

• [MAJOR] style ConfigItem.java.DOJO_SOURCE_DEBUG: Javadoc /** Enable the dojo source code debugging */ at 3-space indent, but public static final ConfigItem<Boolean> DOJO_SOURCE_DEBUG at 4-space indent — inconsistent by 1 space.

• [MAJOR] style p2j.js.doDialogLayout: Function keyword at 17 spaces, but sibling computeDialogSize at 16 spaces within the same try block. Off by 1 space.

• [MAJOR] style p2j.js.ModalDialog: Object literal {"noTitle" : false, "closable" : false} uses space before colon, while multi-line object literals in the same file (e.g. design: 'headline', region: 'center') do not. Three different colon spacing conventions coexist.

• [MAJOR] style p2j.keyboard.js.onkeypress (CHUI): New if (p2j.isDialogDisplayed()) guard at 11 spaces indent, while surrounding code uses 10 spaces. Inconsistent by 1 space.

• [MAJOR] style p2j.mouse.js.processMouseMove: New code at 10 spaces indent where the GCD 3-space rule dictates 9 spaces (3 levels deep: function → inner function → body = 3 x 3).

• [MAJOR] functional ModalDialog.setTitle: setTitle("") does not clear the previous title because empty string is falsy in JavaScript, so dojoDialog.set("title", title) is never called. The previous title text persists.

• [MAJOR] functional ModalDialog.close: dialogs.delete(that.id) moved to AFTER the onClose listener. During the onClose callback, isDialogDisplayed() still returns true — all keyboard/mouse handlers block events even though the dialog is logically closing.

• [MAJOR] functional ModalDialog.close: Calls that.dojoDialog.hide() directly, bypassing the public hide() method. The onHide listener is never fired when close() disposes a visible dialog.

• [MAJOR] functional ModalDialog.close: Not awaited by several callers (p2j.socket.js, p2j.sso_reauth.js, p2j.js). The close operation chains on operationGuard but callers may null the dialog reference while the async chain is still pending.

• [MAJOR] functional ModalDialog.hide: No that.disposed guard before accessing that.dojoDialog. Works accidentally (null check on dojoDialog serves as proxy) but fragile.

• [MAJOR] functional ModalDialog.addTrigger: For dialogs with closable: false (all system dialogs), the onCancel listener is stored but dojoDialog.onCancel is only overridden when closable === true. The stored listener is never invoked — dead code.

• [MAJOR] functional ModalDialog.hide vs close: dialogs.delete() ordering inconsistency. hide() deletes from dialogs BEFORE invoking onHide listener. close() deletes AFTER invoking onClose listener. During onHide, isDialogDisplayed() returns false; during onClose, it returns true.

• [MAJOR] functional p2j.socket.js.ping.checkLostPings: Connection-loss detection timing changed from ~maxIdleTime to ~maxIdleTime/4. New maxLostPings formula with watcher at pingPongInterval/2 detects loss up to 4x faster. Risks premature reconnect before the server declares the session dead.

• [MAJOR] functional p2j.socket.js.connectivityTimer.idleTimer: Changed from Math.min(watchdogTimeout, maxIdleTime) to Math.max(watchdogTimeout, maxIdleTime). This reverses the effective timeout logic — previously the tighter bound constrained timers; now the looser bound applies.

• [MAJOR] functional ModalDialog.onCancel: that.hide() is not awaited in the Dojo dialog's onCancel callback. If the onCancel listener itself invokes close(), the two serialized operations race.

• [MAJOR] functional ModalDialog.createDojoDialog.dojoDialog.onCancel: that.hide() is fire-and-forget — the onCancel function does not return/await it, and Dijit does not await onCancel. However, hide() catches errors internally via try/catch, so unhandled rejection risk is mitigated.

• [MAJOR] functional p2j.socket.js: pingPongInterval floored at 100ms via Math.max(cfg.socket.pingPongInterval, 100). If the configured value is non-numeric or missing, Math.max(undefined, 100) yields NaN, causing silent timer failures.

• [MINOR] style p2j.socket.js: Math.trunc( maxIdleTime / (2 * pingPongInterval)) has a space after Math.trunc( — violates "none inside parentheses" rule.

• [MINOR] style p2j.clipboard.js: Trailing space before ) on excludedContainer.contains(evt.target)) .

• [MINOR] style p2j.screen.js.chooseFiles: const okButtonLabel = p2j.i18n.gettext("OK") — double space after =.

• [MINOR] style p2j.client.css: padding-top: 0px — zero values should omit the px unit.

• [MINOR] style p2j.client.css: padding: 1px 0px 1px 0px — zero values should omit the px unit.

• [MINOR] performance AbstractModulesHandler.deliverModule: The synchronized method serializes all concurrent module resource requests — primary contention source preexisting this change. Debug mode adds a fallback path (1 extra mapToResource + 1 extra getResource) when the uncompressed resource is absent.

• [MINOR] performance ModalDialog: Every method wraps its body in operationGuard.then(async () => {...}), allocating a new Promise + async function closure per invocation. Adds per-call garbage pressure for operations called in rapid succession.

• [MINOR] performance ModalDialog.computeDialogSize.doDialogLayout: Forces 1 synchronous layout reflow per show via borderContainer.domNode.style.height = 'auto' combined with scrollHeight/offsetHeight reads.

• [MINOR] performance p2j.keyboard.js.p2j.mouse.js: p2j.isDialogDisplayed() (O(1)) added to every keydown/keypress/keyup handler (6 locations) and mousemove handler (1 location). In highest-frequency event path.

• [MINOR] performance DojoToolkitHandler.mapToResource: In debug mode, every .js request hits two String.endsWith checks then String.concat(".uncompressed.js"), allocating an intermediate string per request.

• [MINOR] security p2j.screen.js.chooseFiles.onComplete: Remaining innerHTML usage with p2j.i18n.ngettext() (lines 8205, 8212) inconsistent with the textContent fix applied to onError. Only numeric parameters interpolated today (no current exploit path), but should use textContent for consistency.

• [MINOR] security p2j.socket.js.checkLostPings: WebSocket close reason string includes runtime values lostPings and maxLostPings transmitted in-band via ws.close(), exposing internal configuration to the server endpoint.

• [MINOR] functional ModalDialog.setTitle: noTitle logic for empty string is inverted. setTitle("") evaluates to false (since "" is falsy), so title bar is shown empty rather than hidden.

• [MINOR] functional ModalDialog.constructor: When hidden=false, show() is called without await. Since show() chains through async operationGuard, dialog may not be fully shown when the constructor returns. All current callers pass hidden=true, but latent.

• [MINOR] functional ModalDialog.createDojoDialog: 30-second timeout does not set that.disposed on rejection. Callers checking isDisposed() see a non-disposed dialog that is actually unusable.

• [MINOR] functional ModalDialog.constructor: Unnecessary textDiv DOM. A <div class="text"> is created even for dialogs that use setContent() (which clears contentDiv).

• [MINOR] functional p2j.screen.js.chooseFiles.onComplete: Old short-circuited closeAction() for !multiple files. New code always shows the result dialog for 5 seconds — visible UX change.

• [MINOR] functional p2j.screen.js.chooseFiles.onProgress: Old code checked event.type == "load"; new code unconditionally calls actionResultsDialog.show(). Changed from completion-triggered to progress-triggered.

• [MINOR] functional p2j.screen.js.chooseFiles: me.processMouse changed from direct parameter to hardcoded false. Mouse processing always disabled during file selection regardless of direct flag.

• [MINOR] functional p2j.screen.js.chooseFiles: sendFileChoose() called before closeAction() completes in onError and onComplete handlers. Server notification fires before dialog cleanup finishes.

• [MINOR] functional p2j.screen.js.activateWindow: Returns false instead of undefined. Both falsy but return type changed from implicit void to explicit boolean.

• [MINOR] functional processMouseMove: isDialogDisplayed() guard placed before sendMouseMove check. When modal dialog displayed, sendMouseMove=true one-shot is silently consumed.

• [MINOR] functional Utils.removeEscapedDoubleQuotes: Only handles backslash-escaped double quotes (\"), not " XML entities.

• [MINOR] functional Utils.removeEscapedDoubleQuotes: Does not trim whitespace after quote removal. Input " text " produces " text " (leading/trailing spaces preserved).

I found interesting issue that AI can makes a change in the review from its previous note to the opposite note.

[MAJOR] functional ModalDialog close: dialogs.delete(that.id) runs before onClose listener fires. During listener execution, isDialogDisplayed() returns false so keyboard events are not blocked.

to

[MAJOR] functional ModalDialog.hide vs close: dialogs.delete() ordering inconsistency. hide() deletes from dialogs BEFORE invoking onHide listener. close() deletes AFTER invoking onClose listener. During onHide, isDialogDisplayed() returns false; during onClose, it returns true.

That is probably the matter of the agreement of how to use listeners and when they are invoked.

Finally I realized that onHide and onClose listeners should be protected and do not send any key and mouse events to the business logic so I was confused by the review committed rev 16594 (10915a).

FWD Code Review — Curated Findings¶

Summary¶

20 files modified (2957 line diff). The prior CRITICAL finding (sso_reauth.js overrideDialogStyle API break) and 6 MAJOR functional issues were fixed. 39 issues remain across style, functional, performance, and security.

Fixed issues include: sso_reauth.js overrideDialogStyle API break; ModalDialog.setTitle("") clearing; dialogs.delete ordering (moved before onClose); hide/close dialogs.delete consistency; ping/checkLostPings timing clarified; connectivityTimer/idleTimer min→max reversal intentional; onProgress restructured.

Findings¶

• [MAJOR] style p2j.js: History entry 034 first date (20251220) predates entry 033 (20260326). Not chronological.

• [MAJOR] style p2j.socket.js: History entry 083 first date (20251220) predates entry 082 (20260326). Not chronological.

• [MAJOR] style p2j.screen.js: History entry 102 first date (20251220) predates entries 100/101 (20260401/20260504). Not chronological.

• [MAJOR] style Utils.java: History entry 111 first date (20260206) predates entry 110 (20260402). Not chronological.

• [MAJOR] style p2j.mouse.js.processMouseMove: New isDialogDisplayed guard at 10-space indent; existing code at same nesting level uses 9 spaces.

• [MAJOR] style p2j.keyboard.js.onkeypress (CHUI): Dialog guard at 10-space indent; surrounding code at same level uses 9 spaces.

• [MAJOR] style p2j.socket.js.serverUnavailableDialogEscapeTrigger: try-catch block inside function body at 0 indent (commented debug lines also at 0 indent).

• [MAJOR] functional ModalDialog.close: Bypasses hide() — calls dojoDialog.hide() directly rather than that.hide(). The onHide listener registered via addTrigger("onHide", ...) is never invoked when close() disposes a visible dialog.

• [MAJOR] functional ModalDialog.setTitle: that.noTitle = title ? title.length 0 : false — when title="", the falsy branch yields false, so the title bar is shown (via hideTitle(false)) rather than hidden. The title.length0 branch is unreachable for any real input.

• [MAJOR] functional p2j.screen.js.chooseFiles.onComplete: The if (!multiple) { closeAction(); return; } short-circuit was removed. Single-file uploads now wait the full 5-second delayForCloseAction before closing, regressing from immediate-close behavior.

• [MAJOR] functional ModalDialog.close: Not awaited by most callers (loadingDialog.close() in p2j.js, restoreConnectionDialog.close() and serverUnavailableDialog.close() in socket.js, alertDialog.close() in screen.js, reauthDialog.close() in sso_reauth.js). None await the returned promise.

• [MAJOR] functional p2j.mouse.js.processMouseMove: isDialogDisplayed() guard at line 362 fires BEFORE the !sendMouseMove check at line 367, defeating the comment's stated purpose that mouse moves must be sent for dialog-boxes after SET_CURSOR.

• [MAJOR] functional p2j.screen.js.chooseFiles: me.processMouse changed from direct to hardcoded false, disabling all mouse event processing during file selection regardless of the direct flag.

• [MAJOR] performance AbstractModulesHandler.deliverModule: Method is synchronized, serializing all module requests through a single lock. Every page load with 10-50+ JS module requests contends on this lock.

• [MINOR] style ConfigItem.java.DOJO_SOURCE_DEBUG: Field continuation uses 7-space indent, while neighboring JS_DEBUG_LOGGING uses a different continuation style. Inconsistent.

• [MINOR] style p2j.socket.js: Math.trunc( maxIdleTime... — space after (, should be removed.

• [MINOR] style p2j.clipboard.js: Trailing whitespace after ) on line 283.

• [MINOR] style p2j.screen.js.chooseFiles: const okButtonLabel = p2j... — double space after =.

• [MINOR] style p2j.js: catch(e) should be catch (e) (space before condition).

• [MINOR] style p2j.screen.js: catch(e) should be catch (e) (space before condition).

• [MINOR] style p2j.client.css: padding-top: 0px — zero values should omit px.

• [MINOR] style p2j.client.css: padding: 1px 0px 1px 0px — zero values should omit px.

• [MINOR] performance ModalDialog: Every method (show, hide, close, setContent, setTextContent, setButtons, setFooter, setTitle, overrideDialogStyle, overrideTitleStyle, updateLayout) chains .then() on operationGuard, allocating a new Promise per call.

• [MINOR] performance ModalDialog.computeDialogSize: Writes style.height='auto' then reads offset/scroll properties, forcing 1 synchronous reflow per call.

• [MINOR] performance p2j.keyboard.js/p2j.mouse.js: isDialogDisplayed() called in all GUI and CHUI keyboard handlers plus processMouseMove — expanded hot-path footprint (dialogs.size > 0, O(1), but in highest-frequency events).

• [MINOR] performance DojoToolkitHandler.mapToResource: concat(".uncompressed.js") + replace() = 2 string allocations per request when debugMode enabled.

• [MINOR] performance p2j.socket.js.ping: setTimeout fires at pingPongInterval >> 1, doubling the healthy-path checkLostPings callback rate vs old pingPongInterval.

• [MINOR] performance ModalDialog.show: Calls doDialogLayout() on already-visible dialog, forcing unnecessary reflow per repeated show().

• [MINOR] security p2j.screen.js.chooseFiles.onComplete: Remaining innerHTML usage at lines 8205 and 8212 for resultDiv. onError was correctly changed to textContent, but onComplete was not — inconsistent.

• [MINOR] security p2j.socket.js.checkLostPings: closeWebSocketSafely() at line 5876 transmits internal config values (lostPings, maxLostPings) in the WebSocket close reason frame, observable by network intermediaries.

• [MINOR] functional ModalDialog.addTrigger: For dialogs with closable: false (all system dialogs), the onCancel listener stored via addTrigger("onCancel", handler, false) is stored in that.listeners, but dojoDialog.onCancel is only bound when that.closable is true. The listeners are unreachable dead code.

• [MINOR] functional ModalDialog.onCancel: that.hide() called without await inside dojo dialog's onCancel. hide() is async; Dojo will not await the returned promise.

• [MINOR] functional ModalDialog.hide: No that.disposed guard. Unlike show() and close(), hide() does not explicitly check disposed state. Works accidentally via !that.dojoDialog null check.

• [MINOR] functional p2j.socket.js: Math.max(cfg.socket.pingPongInterval, 100) — if configured value is undefined, returns NaN, breaking all ping/pong timing calculations downstream.

• [MINOR] functional ModalDialog.constructor: When hidden=false, show() called without await. Constructor is synchronous; show() is async. All current callers pass hidden=true, but latent.

• [MINOR] functional ModalDialog.createDojoDialog: 30-second timeout handler rejects the deferred but does not set that.disposed = true, leaving the dialog in an ambiguous non-disposed but unusable state.

• [MINOR] functional ModalDialog.constructor: Initial textDiv created inside createDojoDialog is later replaced by setTextContent/setContent, creating wasted DOM nodes.

• [MINOR] functional Utils.removeEscapedDoubleQuotes: Only strips \" wraps, not " HTML entities (another common wrapping for XML-configured string values).

• [MINOR] functional Utils.removeEscapedDoubleQuotes: Does not trim() the result. Input " value " produces " value " with leading/trailing spaces.

At the moment of writing the code this part was intentional:

[MAJOR] functional ModalDialog.close: Bypasses hide() — calls dojoDialog.hide() directly rather than that.hide(). The onHide listener registered via addTrigger("onHide", ...) is never invoked when close() disposes a visible dialog.

Code review 10915a revisions 16559..16591

• [MAJOR] functional chooseFiles: onError reads actionResultsDialog.files.length, but actionResultsDialog.files is only assigned inside onChange. If an upload error fires before onChange (e.g., immediate server rejection in direct mode), actionResultsDialog.files is undefined and the ngettext call throws.

Test disconnet dialogs still work well under various network conditions (slow/fast network, higher latencies).

• [MAJOR] functional startConnect (p2j.socket): restoreConnectionDialog (the "lost, trying to reconnect" UI) is given the title "Server Unavailable", the same title used for the terminal serverUnavailableDialog. This is misleading: "Server Unavailable" implies a terminal state, while the restore dialog represents a transient retry state. A title such as "Connection Lost" or "Reconnecting" would be more accurate.

• [MAJOR] functional doRedirectToLogoutPage: In the window.fetch(logoutPage).catch(...) branch the new code clears hideRestoreConnectionDialog and calls restoreConnectionDialog.hide() before serverUnavailableDialog.show(). The two calls schedule onto separate operationGuard chains, so there is no guarantee restoreConnectionDialog is hidden before serverUnavailableDialog becomes visible. For a brief window both dialogs may overlap in the DOM, especially with the legacy updateZIndex overlay logic now removed. Please double check this.

• [MAJOR] functional startupServer (EmbeddedWebServerImpl): When DOJO_SOURCE_DEBUG is enabled, the embedded web server serves uncompressed Dojo source files without any production-mode gating, audit log, or startup warning. Consider logging a WARNING when debug mode is activated.

Please double check the code indentations are at 3 characters, I noticed some places where 4 were used.

Yes, this one restoreConnectionDialog.setTitle(p2j.i18n.gettext("Server Unavailable")); can be removed because the reconnection dialog uses this title restoreConnectionDialog.setTitle(p2j.i18n.gettext("Attempt #%1",connectionTries));. Don't understand why this line was here.

This one

• [MAJOR] functional p2j.mouse.js.processMouseMove: isDialogDisplayed() guard at line 362 fires BEFORE the !sendMouseMove check at line 367, defeating the comment's stated purpose that mouse moves must be sent for dialog-boxes after SET_CURSOR.

is related to dialogs created by the converted code. From the other side ModalDialogs should not impact the converted code that implements the application business logic so key and mouse events should not be sent to the java web client.

Sergey Ivanovskiy wrote:

Stefanel, could you help with the test case to check the reauth dialog style from p2j.sso_reauth.js?

Yes, I can help. Is 10915a ready to be tested with this or should I wait for other changes to be committed?

Yes, the current code looks suitable but there can be issues with ModalDialog usages that needs to be checked. Actually, dialogs methods show(), hide(), setTitle() and etc. are asynchronous but their execution order is supported.

This one

[MAJOR] functional doRedirectToLogoutPage: In the window.fetch(logoutPage).catch(...) branch the new code clears hideRestoreConnectionDialog and calls restoreConnectionDialog.hide() before serverUnavailableDialog.show(). The two calls schedule onto separate operationGuard chains, so there is no guarantee restoreConnectionDialog is hidden before serverUnavailableDialog becomes visible. For a brief window both dialogs may overlap in the DOM, especially with the legacy updateZIndex overlay logic now removed. Please double check this.

I checked the code and can confirm that the order of ModalDialog method calls is supported and only when one method body is completed, the next method call starts its execution.

If it is important for the caller of ModalDialog to wait until its method is completed, then await construction should be used await dialog.show() or await dialog.hide().

Stefanel, thank you for help with this dialog checking! If we require Close dialog button (X) and the title, then

reauthDialog = new p2j.ModalDialog('p2j-sso-reauth-dialog', document.body, '', '', true, {"noTitle" : false, "closable" : true}); 
reauthDialog.setTitle(...);

To remove the dialog title bar we need to set this option value noTitle : true

reauthDialog = new p2j.ModalDialog('p2j-sso-reauth-dialog', document.body, '', '', true, {"noTitle" : true, "closable" : false}); 

I think that false options can be omitted so

reauthDialog = new p2j.ModalDialog('p2j-sso-reauth-dialog', document.body, '', '', true, {"closable" : true}); 

reauthDialog = new p2j.ModalDialog('p2j-sso-reauth-dialog', document.body, '', '', true, {"noTitle" : true});

Committed revision 16600 (10915a) fixed

[MAJOR] functional startupServer (EmbeddedWebServerImpl): When DOJO_SOURCE_DEBUG is enabled, the embedded web server serves uncompressed Dojo source files without any production-mode gating, audit log, or startup warning. Consider logging a WARNING when debug mode is activated.

Hynek, what was the reason that this code was placed at the top before logKeyEvent(evt, "keypress"); and guards?

       * ALT key is the modifier which sets the key code's 10th bit.  If this is determined to be
       * pressed together with the key, than bit 10 will be set and this key will be pushed to the
       * {@link SwingKeyboardReader#queue}, but only if the key code's label is not the empty string.
       * 
       * @param    {KeyboardEvent} evt
       *           The browser generated keyboard event.
       */
      function onkeypress(evt)
      {
         if (evt.key === undefined || evt.key == "Unidentified" || evt.key == "")
         {
            if (evt.charCode !== undefined && evt.charCode !== 0)
            {
               var keyCode = getKeyCode(evt);
               var newKey = String.fromCharCode(evt.charCode);
               var newEvt = p2j.createKeyEvent("keypress", evt.bubbles, evt.cancelable, evt.view, 
                     evt.ctrlKey, evt.altKey, evt.shiftKey, evt.metaKey, 
                     keyCode, evt.charCode, newKey, evt.code);
               onkeypress(newEvt);
               return;
            }
         }
         logKeyEvent(evt, "keypress");

         if (rejectKeyEvent(evt))
         {
            return;
         }
         if (p2j.isDialogDisplayed())
         {
            return false;
         }

Committed rev 16603(10915a) should fix most found issues (those that can be fixed) except

• [MAJOR] functional p2j.screen.js mouseDownHandler, mouseUpHandler, mouseClickHandler, mouseContextMenuHandler: When isDialogDisplayed() is true, these handlers call p2j.consumeEvent(evt) (which calls stopPropagation/preventDefault but does not halt execution) and then continue through dispatchEvent → sendMouseEvent, sending mouse events to the server behind a modal dialog. Only mouseMoveHandler correctly returns early after consuming the event.

that can be addressed after we discussed #10915-178.

Sergey Ivanovskiy wrote:

Hynek, what was the reason that this code was placed at the top before logKeyEvent(evt, "keypress"); and guards?

This was for the cases when evt.key is not defined. key itself is deprecated and in some environments it is not set anymore. One such case is Lyra built on top of Playwright.

Hynek Cihlar wrote:

Sergey Ivanovskiy wrote:

Hynek, what was the reason that this code was placed at the top before logKeyEvent(evt, "keypress"); and guards?

This was for the cases when evt.key is not defined. key itself is deprecated and in some environments it is not set anymore. One such case is Lyra built on top of Playwright.

In this case we should guard this code by lyra flag.

Hynek Cihlar wrote:

Sergey Ivanovskiy wrote:

Hynek, what was the reason that this code was placed at the top before logKeyEvent(evt, "keypress"); and guards?

This was for the cases when evt.key is not defined. key itself is deprecated and in some environments it is not set anymore. One such case is Lyra built on top of Playwright.

MDN does not deprecate key field!

https://developer.mozilla.org/en-US/docs/Web/API/KeyboardEvent/key

Sergey Ivanovskiy wrote:

Hynek Cihlar wrote:

Sergey Ivanovskiy wrote:

Hynek, what was the reason that this code was placed at the top before logKeyEvent(evt, "keypress"); and guards?

This was for the cases when evt.key is not defined. key itself is deprecated and in some environments it is not set anymore. One such case is Lyra built on top of Playwright.

MDN does not deprecate key field!

https://developer.mozilla.org/en-US/docs/Web/API/KeyboardEvent/key

Sorry for the confusion, Playwright (in Lyra) only fills charCode, that is why key must be calculated. I will add this explanation in a code comment in 10860b.

OK. I added the guide for this code. Could you review the last code rev 16620. I tried to fix the most issues.

I would reject them all.

Sergey Ivanovskiy wrote:

These issues are shuffled from AI review to another one. Could you select the ones that can be fixed?

These were genuine from the latest state of the code. I checked each of them and they seem legit to me.

Hynek Cihlar wrote:

Sergey Ivanovskiy wrote:

These issues are shuffled from AI review to another one. Could you select the ones that can be fixed?

These were genuine from the latest state of the code. I checked each of them and they seem legit to me.

May be you are correct. Those seem to me like shuffled old issues because of asynchronous code. Will try to fix them.

Sergey Ivanovskiy wrote:

May be you are correct. Those seem to me like shuffled old issues because of asynchronous code. Will try to fix them.

Actually I never do that. I always do review of the most recent code state and always check item by item removing anything, which is a non-issue. Obviously I can make mistakes as I typically don't runtime-check everything.

OK. Thank you. I see.

The first issue I should discuss here

               await that.dojoDialog.show();
               setVisible(true);
               dialogs.add(that.id);

               dialogs.add(that.id);
               await that.dojoDialog.show();
               setVisible(true);

The second issue is really came from my changes in which I did the dialog without closed button x but forgot to remove addTrigger("onCancel",.)

The third issue looks bad because adding await requires to add async marker the hosted function.

[MAJOR] functional p2j.socket.doRedirectToLogoutPage: restoreConnectionDialog.hide() and serverUnavailableDialog.show() are issued back-to-back on two separate dialog instances, each with its own operationGuard. The chains race; serverUnavailableDialog may enter DialogLevelManager._dialogStack and grab the underlay before restoreConnectionDialog finishes fading out, leaving the underlay tied to the wrong dialog. Fix: await restoreConnectionDialog.hide() before invoking serverUnavailableDialog.show(). I think it makes sense to add the await there.

The 4th issue is similar to 3d.

[MAJOR] functional ssoReauth.handleSsoReauth: reauthDialog.show() is asynchronous (chained on operationGuard) but btnRelogin.focus() is called synchronously right after. At that moment createDojoDialog has not yet resolved, the panel is still detached, and btnRelogin.focus() is a no-op. Initial focus depends entirely on dijit's autofocus. Fix: await reauthDialog.show() before focusing, or register an onShow listener that focuses btnRelogin.

This one is simple but should be discussed, The proposed solution is to remove title.

[MAJOR] functional ssoReauth.handleSsoReauth: new p2j.ModalDialog('p2j-sso-reauth-dialog', document.body, '', '', true) omits the titleOption argument so the constructor defaults to noTitle: false. The dijit Dialog renders an empty title bar above the SSO panel — UI regression vs the previous custom-HTML dialog. Fix: pass {noTitle: true, closable: false}.

The 6th issue must be discussed because I do not know why it should be consumed.

[MAJOR] functional CHUIKeyboardReader.onkeyup: The new isDialogDisplayed() early return prevents the handler from calling consumeEvent/preventDefault, so ESC/arrow keys hitting the CHUI canvas while a dialog is shown may trigger browser-native behavior (page scrolling, history back, etc.). Consume the event before returning, or rely on the dialog's own consumption. Please check this one.

Sergey Ivanovskiy wrote:

This one is simple but should be discussed, The proposed solution is to remove title.

Is there title in trunk? If not, just remove the title bar from the dialog in this case so that it doesn't look like something's missing.

Sergey Ivanovskiy wrote:

The 6th issue must be discussed because I do not know why it should be consumed.

This item is about default browser behavior. Like when you press TAB key, we don't want the focus leave FWD running app in the browser's address bar.

The 7th issue is due to unclear code that was from Playwright and Lyra. I proposed to add the guard here. If this one p2j.lyra is not generic, then it must be added another one.

[MAJOR] functional GUIKeyboardReader.onkeypress: The fallback that synthesizes a key event when evt.key = undefined || evt.key "Unidentified" || evt.key "" was wrapped in if (p2j.lyra). Non-Lyra clients (Playwright tests, Safari, etc.) now fall through to keyPressEmitted = true; lastPressedKey = evt.key Pl; with undefined/empty key. Either remove the p2j.lyra gate or document why the synthesis is Lyra-only and add an equivalent fallback for the standard path. Please don't limit this to Lyra. If for any reason we run a test directly with Playwright this workaround won't work.

At least we need to add comments before evt.key = undefined || evt.key "Unidentified" || evt.key ""

Th 8-th issue should be discussed because we should not need to send mouse and key events to the business layer. It is supposed that events are performed in UI thread so the dialog is directed to show sequentially. The modal dialog is shown above the application view and it does not matter which state has the business cursor at the moment before the modal dialog appears.

[MAJOR] functional MouseHandler.processMouseMove: The new if (p2j.isDialogDisplayed()) return; is placed BEFORE the !p2j.screen.canProcessOsEvent(evt, win) && !sendMouseMove check, defeating the documented force-send pathway used by setGlobalCursor where me.sendMouseMove = true forces one mousemove through. Even with sendMouseMove true, the event is dropped when any FWD modal dialog is up. Gate the new check with && !sendMouseMove if force-send must still pass. Please test how modal dialogs interact with a global cursor active.

The 9-th issue is quite clear.

[MAJOR] functional EmbeddedWebHandler.<init>: Still constructs new DojoToolkitHandler(TARGET_EMBEDDED) via the legacy constructor (forces debugMode=false). The new DOJO_SOURCE_DEBUG flag has no effect on the embedded web app. Either plumb the flag here or document the limitation in the ConfigItem javadoc.

Sergey Ivanovskiy wrote:

At least we need to add comments before evt.key = undefined || evt.key "Unidentified" || evt.key ""

Please see #10915-186.

OK. I will leave this and remove p2j.lyra guard.

Sergey Ivanovskiy wrote:

Th 8-th issue should be discussed because we should not need to send mouse and key events to the business layer. It is supposed that events are performed in UI thread so the dialog is directed to show sequentially. The modal dialog is shown above the application view and it does not matter which state has the business cursor at the moment before the modal dialog appears.

[MAJOR] functional MouseHandler.processMouseMove: The new if (p2j.isDialogDisplayed()) return; is placed BEFORE the !p2j.screen.canProcessOsEvent(evt, win) && !sendMouseMove check, defeating the documented force-send pathway used by setGlobalCursor where me.sendMouseMove = true forces one mousemove through. Even with sendMouseMove true, the event is dropped when any FWD modal dialog is up. Gate the new check with && !sendMouseMove if force-send must still pass. Please test how modal dialogs interact with a global cursor active.

The question is should the global cursor be set to the default when a non-legacy modal dialog is displayed? I would say it should, but then after it is dismissed it should be reverted back to the previously set global cursor. This is not a major issue, but it surfaced because the changes modify the previous behavior.

Sergey Ivanovskiy wrote:

The second issue is really came from my changes in which I did the dialog without closed button x but forgot to remove addTrigger("onCancel",.)

This can be discussed. I supposed initially that these dialogs have no Close x button. Should they have this button? At least the server unavailable dialog can have this button.

Hynek Cihlar wrote:

Sergey Ivanovskiy wrote:

Th 8-th issue should be discussed because we should not need to send mouse and key events to the business layer. It is supposed that events are performed in UI thread so the dialog is directed to show sequentially. The modal dialog is shown above the application view and it does not matter which state has the business cursor at the moment before the modal dialog appears.

[MAJOR] functional MouseHandler.processMouseMove: The new if (p2j.isDialogDisplayed()) return; is placed BEFORE the !p2j.screen.canProcessOsEvent(evt, win) && !sendMouseMove check, defeating the documented force-send pathway used by setGlobalCursor where me.sendMouseMove = true forces one mousemove through. Even with sendMouseMove true, the event is dropped when any FWD modal dialog is up. Gate the new check with && !sendMouseMove if force-send must still pass. Please test how modal dialogs interact with a global cursor active.

The question is should the global cursor be set to the default when a non-legacy modal dialog is displayed? I would say it should, but then after it is dismissed it should be reverted back to the previously set global cursor. This is not a major issue, but it surfaced because the changes modify the previous behavior.

I do not know how to do it because the cursor must be controlled by the business logic. This part is not elaborated. Especially when we change the global cursor trying to restore some intermediate state when the non-legacy dialog is completed.

I do not know how to do it because the cursor must be controlled by the business logic. This part is not elaborated. Especially when we change the global cursor trying to restore some intermediate state when the non-legacy dialog is completed.

Hynek, I meant that the cursor position and the restored cursor can be unrelated. Should we do this?

I committed rev 16661 (10915a) and started code review. I did not do anything with these issues: issue 6, 8, 9.

Sergey Ivanovskiy wrote:

I do not know how to do it because the cursor must be controlled by the business logic. This part is not elaborated. Especially when we change the global cursor trying to restore some intermediate state when the non-legacy dialog is completed.

Hynek, I meant that the cursor position and the restored cursor can be unrelated. Should we do this?

Ideally the behavior should not change from trunk. But the related code changed, so I suggest you test to change global cursor, display a dialog and then close it. Compare the behavior against trunk.

Sergey Ivanovskiy wrote:

Sergey Ivanovskiy wrote:

The second issue is really came from my changes in which I did the dialog without closed button x but forgot to remove addTrigger("onCancel",.)

This can be discussed. I supposed initially that these dialogs have no Close x button. Should they have this button? At least the server unavailable dialog can have this button.

I don't think close title button is necessary here as the dialog buttons already provide the required ui flow.

I have these critical issues rooted in the legacy code. Could those be fixed in new optimization task?

• [CRITICAL] performance AbstractModulesHandler.deliverModule: The method is synchronized, serializing every dojo/web-resource request through a single monitor per handler instance. The lock exists only because AbstractHandlerCommons.buffer is a shared mutable instance field; making the buffer a local in sendResponse would remove the need for synchronization. With DOJO_SOURCE_DEBUG=true each miss triggers a second ClassLoader.getResource under the same lock.
• [CRITICAL] performance AbstractHandlerCommons.sendResponse: The shared private byte[] buffer = new byte[BUFFER_SIZE] instance field forces the caller to synchronize, becoming the choke point for the module handlers. Allocate the buffer per-call (or use is.transferTo(os)) so concurrent module requests can run in parallel.
• [CRITICAL] performance AbstractModulesHandler.deliverModule: No ETag/Last-Modified/If-None-Match handling for dojo or webjar modules — every /dojo-toolkit/... request returns a full 200 body, while /client and /common via HashedResourceHandler correctly emit ETags and return 304s. Under DOJO_SOURCE_DEBUG=true each (large) uncompressed module is re-downloaded on every page load.

I ran several rounds of the review. The process yielded highly useful insights because the codebase is both asynchronous and generic, presenting numerous complex use cases. However, there is a distinct downside to this approach: later iterations continuously uncover new issues, likely because the LLM is following the statistically high-probability continuation of the ongoing review context.

Frankly, this process became exhausting due to the immense cognitive load required to trace every asynchronous execution path and account for all edge cases. On the positive side, it served as great training. Ultimately, I believe that if code changes were tightly scoped to a few minor fixes per iteration, passing the AI review would be a much easier and more manageable process.

Committed rev 16625. There are edge cases that still not fixed.

Sergey Ivanovskiy wrote:

I have these critical issues rooted in the legacy code. Could those be fixed in new optimization task?

• [CRITICAL] performance AbstractModulesHandler.deliverModule: No ETag/Last-Modified/If-None-Match handling for dojo or webjar modules — every /dojo-toolkit/... request returns a full 200 body, while /client and /common via HashedResourceHandler correctly emit ETags and return 304s. Under DOJO_SOURCE_DEBUG=true each (large) uncompressed module is re-downloaded on every page load.

This is related with #11452, which I have skipped handling caching for dojo toolkit because it required more work to do, which was not related to the issue from task.

OK. I am trying to pass this review as soon as possible due to incoming bugs #11412 and #11485 that were fixed in 10915a.

I asked to prepare proposed solutions to each of the reported issues in the separated file corresponding each major issue in the ai review after the issue description at the top.

The results look interesting. Can they be posted? I would apply some of them because their looks are reasonable.

Sergey Ivanovskiy wrote:

Frankly, this process became exhausting due to the immense cognitive load required to trace every asynchronous execution path and account for all edge cases. On the positive side, it served as great training. Ultimately, I believe that if code changes were tightly scoped to a few minor fixes per iteration, passing the AI review would be a much easier and more manageable process.

It is important to recognize, which identified points can be safely skipped. The review is currently quite sensitive and marks even "issues" that practically never surface. For example the performance points, technically they may be correct, but do they surface when the dialogs open? If not I would just skip them.

Committed revision 16626 (10915a) is a good candidate for the review. Applied almost patches and running the review now.

Also I tested these changes with testcases.

Sergey Ivanovskiy wrote:

OK. Please look at this issue that is not related to my diff.

If it is not related to your changes and also present in trunk, please create new Redmine issue.

Hynek Cihlar wrote:

Sergey Ivanovskiy wrote:

OK. Please look at this issue that is not related to my diff.

If it is not related to your changes and also present in trunk, please create new Redmine issue.

The source code is in src/com/goldencode/p2j/main/VirtualDesktopWebHandler.java but I am not sure the new bug should be in Runtime Infrastructure or in UI project?

Sergey Ivanovskiy wrote:

If it is not related to your changes and also present in trunk, please create new Redmine issue.

The source code is in src/com/goldencode/p2j/main/VirtualDesktopWebHandler.java but I am not sure the new bug should be in Runtime Infrastructure or in UI project?

Runtime Infrastructure.

Committed rev 16628 got this result

- 0 CRITICAL — the 1 originally proposed (close() deadlock) was challenged and rejected (the postponed close runs after
  locked=false; no self-referential dependency).
  - ~17 MAJOR:
    - Security: WebPageHandler injects admin-controlled strings into JS literals without escaping (the removeEscapedDoubleQuotes
  helper is NOT an XSS sanitizer).
    - ModalDialog: keydown ESC handlers blocked during 30 s Dojo-load window (eager dialogs.add vs wrappedHandler gating on
  isVisible()); postponedActions drain has no try/catch; hide() failure leaves id in dialogs Set; stacked dialogs both fire on ESC
  (no z-order check); non-closable dialogs still close on ESC via dijit's default onCancel; setTitle("") on non-closable overrides
  explicit noTitle:true.
    - chooseFiles: closeButton.onClick bypasses closeAction so dojo connect links / cancel listener / closeTimeout leak.
    - SsoReauth: btnRelogin retry guaranteed to fail (state consumed on first callback); no re-entrancy guard → dijit id collision.
    - Performance: AbstractModulesHandler.deliverModule synchronized + shared buffer + debug-mode doubled lookup.
    - Style: 6 items (header > 110 chars, "withot" typo, brace style, missing space before (, inline braces, awkward warning
  grammar).
  - ~30 MINOR: postMessage nonce, dialog state interleavings, setContent dijit widget leak, addTrigger silent overwrite/drop,
  removeTrigger not exposed, processMouse permanently false on edge cases, modifier-only key dismisses ConnectionRestored notice,
  btnLogout hang on stuck close, query parsing duplicates, IME "Process" filter asymmetry, AbstractModulesHandler binary compat
  break, mapToResource debug-mode doubled lookup + no .. defense, etc.
  - Rejected: the deadlock claim; show→hide re-add race (operationGuard serializes); chooseFiles me.processMouse=false "regression" 
  (pre-existing); actionResultsDialog over selectUploadFilesDialog stacking (hide runs first); CHUI keyPressEmitted stale (resets
  are in onkeydown, fire on every event); SsoReauth countdownInterval double-arming (cleanup clears it); pingPongWatcher
  cadence-doubling (misread timer semantics); evt.repeat stuck-key scenario (browser doesn't set repeat on keyup).

  Notable: the postMessage nonce finding (security) was downgraded to MINOR on grounds that exploitation already implies same-origin
   XSS — consider whether defense-in-depth still justifies the nonce.

Sergey Ivanovskiy wrote:

This issue can be discussed. It relates to dialog messages used as translation keys and read from the directory. I postponed this fix.

This can be ignored, the strings are coming from a trusted source - i18n bundles.

OK. Committed revision 16629 (10915a) has opened major issues that can be fixed later in the different tasks.
Running new review.

It seems that 10915a doesn't have CHUI changes. Should I run CHUI regression tests?

Sergey Ivanovskiy wrote:

It seems that 10915a doesn't have CHUI changes. Should I run CHUI regression tests?

There are changes for Web ChUI, that must be regression tested. It's hard to say whether the changes also affect terminal ChUI, probably not, but I would run the ChUI regression tests anyway.

OK. The current code p2j.ModalDialog is over complicated. I was confused by AI review at some moment. Please review only p2j_sso_reauth.js changes.

I finished manual GUI testing of rev 16644 (10915a). The rest tasks are CHUI regression testing and review/testing p2j_sso_reauth.js changes. Stefanel, could you give me instructions to test Hotel Gui and p2j_sso_reauth.js changes using this authentication schema OidcSsoAuthenticator?

Sergey Ivanovskiy wrote:

Stefanel, could you give me instructions to test Hotel Gui and p2j_sso_reauth.js changes using this authentication schema OidcSsoAuthenticator?

I'll check the changes now and I can test while reviewing them if you want.

Thank you. Are there any simulations for Oidc provider for testing?

Sergey Ivanovskiy wrote:

Thank you. Are there any simulations for Oidc provider for testing?

I use this: docker run -p 127.0.0.1:8080:8080 -e KC_BOOTSTRAP_ADMIN_USERNAME=admin -e KC_BOOTSTRAP_ADMIN_PASSWORD=admin quay.io/keycloak/keycloak:26.6.2 start-dev

I tested the changes related to p2j.sso_reauth.js, and they look good and work well for me.

Stefanel, thank you for the review.

The first round of the main regression tests completed with 3 failed tests #7143-1937.

Sergey Ivanovskiy wrote:

The first round of the main regression tests completed with 3 failed tests #7143-1937.

The trunk version has the same failed tests.

Thus 10915a passed ctrl_c regression tests and main regression tests with 3 failed. For the trunk I got the same results.
Hynek, could 10915a be merged into the trunk?

CRITICAL] functional ModalDialog.close: When a `documentListener` keydown trigger handler awaits `dialog.close()`, the call deadlocks permanently. `wrappedChainedListener` sets `that.locked = lock` and then does `await chainedListeners.handle(event)`, which invokes the user handler. The handler awaits `close()`, whose chained `.then` runs `await waitLockAvailable()` — which awaits g`that.locked.promise`. That promise resolves only at `lock.resolve(true)`, which is unreachable until `chainedListeners.handle` returns. Concrete triggers: pressing ESC in `restoreConnectionDialog` (p2j.socket.js `restoreConnectionDialogEscapeTrigger` awaits `restoreConnectionDialog.close()` inside the keydown handler), pressing ESC in `serverUnavailableDialog` (`serverUnavailableDialogEscapeTrigger` awaits `serverUnavailableDialog.close()`), and pressing ENTER in `openPopupDialog` (awaits `openPopupDialog.close()`). The dialog never closes, the redirect to logout / new-window never runs, and all subsequent keyboard events are blocked because `that.locked` is never released.

Hynek, I have no critical issues in my review. This one looks odd because there are no code `wrappedChainedListener` sets `that.locked = lock`. There is no dead lock in wait close().

Sergey Ivanovskiy wrote:

I finished manual GUI testing of rev 16644 (10915a). The rest tasks are CHUI regression testing and review/testing p2j_sso_reauth.js changes. Stefanel, could you give me instructions to test Hotel Gui and p2j_sso_reauth.js changes using this authentication schema OidcSsoAuthenticator?

This was posted yesterday but your code review was 16639.

This is my last review for rev 16444(10915a)

Modified areas: com.goldencode.p2j.main (4 files), com.goldencode.p2j.ui.client.driver.web (10 files), com.goldencode.p2j.ui.client.gui.driver.web (1 file), com.goldencode.p2j.web (3 files), com.goldencode.p2j.util (2 files), plus a translation JSON file

Domains reviewed: fwd-server-infrastructure, fwd-web-gui-driver, fwd-web-services-admin, fwd-web-login-process, fwd-i18n-translation

• [MAJOR] functional p2j.socket.js.checkLostPings: Watcher timeout changed from pingPongInterval to pingPongInterval >> 1 and maxLostPings from Math.trunc(maxIdleTime / pingPongInterval) + 1 to Math.max(Math.trunc(maxIdleTime / (2 * pingPongInterval)), 2). Since lostPings still increments at pingPongInterval rate (idleTimer is unchanged), the wall-clock time to declare the connection lost is roughly halved. Concrete example: maxIdleTime=60s, pingPongInterval=10s — old maxLostPings=7 → ~70s to declare lost; new maxLostPings=3 → ~30s. Trigger: any deployment with a tuned pingPongInterval; lossy networks may see more false-positive disconnects than before. History entry 072 mentions a "maxLostPings logic change" but does not justify the wall-clock reduction.

• [MAJOR] style p2j.js: Entry 034 carries an out-of-order date (20251220 predates entry 033 dated 20260326), and several lines in the multi-line description (lines 82-92) exceed the 110-character limit.

• [MAJOR] style p2j.socket.js: The history block contains a duplicate sequence ID 081 — the old SB 081 and the new SBI 081 (renumbered to 082 in this diff) leave a duplicated 081 entry. Sequence IDs must be successive without duplicates.

• [MAJOR] style ModalDialog.createDojoDialog: The require() callback body indentation does not align with the function keyword; opening brace at line 2005 is at column 9 while the function keyword sits at column 12.

• [MAJOR] style ModalDialog.computeDialogSize: The early-return object literal (lines 2110-2119) uses deep column-aligned padding (keys at column 32) while the second return literal (lines 2150-2158) uses standard 3-space indent. Inconsistent within the same function.

• [MAJOR] style ModalDialog.computeDialogSize: Double blank line at lines 2147-2149 between var dialogHeight = ... and return. Mid-function double blank lines are non-standard.

• [MINOR] functional p2j.screen.js.chooseFiles (direct=true path): chooseFiles unconditionally constructs new p2j.ModalDialog("selectUploadFilesDialog", ...) at p2j.screen.js:7942, whose constructor synchronously calls createDojoDialog → new Dialog({id: that.id}) at p2j.js:2016 which reserves the dijit registry id. For direct=true, dialog.show() is never called, so dialogs.add(that.id) never runs and p2j.isDialogDisplayed() stays false. A second user mouse event (mousedown/mouseup/click each independently call processDirectUpload at p2j.screen.js:1244/1295/1343, gated only by isDialogDisplayed()) before the first dojo.ready callback completes can reach chooseFiles(true, ...) again, producing a second new Dialog({id: "selectUploadFilesDialog"}) that throws on the id collision. No upstream re-entry guard exists.

• [MINOR] style ModalDialog.restoreGlobalCursor: Empty if branch with only a comment as body (p2j.js:2389-2392).

• [MINOR] style ModalDialog.show: Ternary assignment continuation (lines 2415-2417) uses excessive right-padding instead of normal 3-space wrap.

• [MINOR] style ModalDialog.removeAllChildren: Missing space between while keyword and ( at p2j.js:2620.

• [MINOR] style ModalDialog.close: forEach callback brace not aligned with function keyword (p2j.js:3010-3019).

• [MINOR] style WebPageHandler.WebPageKeysProvider: Missing blank line before the new StringBuilder dojoConfig declaration block at line 257.

• [MINOR] style EmbeddedWebServerImpl: Warning message concatenation uses a leading-space-inside-string join rather than trailing-space-on-prior-segment; continuation indent at 6 spaces rather than 3.

• [MINOR] style Utils.removeEscapedDoubleQuotes: Double blank line mid-method between the if (text == null) block and int end = text.length();.

• [MINOR] style p2j.sso_reauth.js.handleSsoReauth: Missing space in catch(e) at line 1845 (should be catch (e)).

• [MINOR] style p2j.socket.js.createOpenPopupDialog: Function declaration ends with }; — top-level function declarations should not have a trailing semicolon.

Hynek, could it be accepted?

Sergey Ivanovskiy wrote:

Sergey Ivanovskiy wrote:

I finished manual GUI testing of rev 16644 (10915a). The rest tasks are CHUI regression testing and review/testing p2j_sso_reauth.js changes. Stefanel, could you give me instructions to test Hotel Gui and p2j_sso_reauth.js changes using this authentication schema OidcSsoAuthenticator?

This was posted yesterday but your code review was 16639.

You are right, I didn't update to the latest.

What about the //p2j.consumeEvent(evt), if not needed, please remove and go ahead with regression testing.

OK. I tested manually all dialogs and Stefanel reviewed/tested Oidc dialogs. CHUI regression tests are passed.

Sergey Ivanovskiy wrote:

OK. I tested manually all dialogs and Stefanel reviewed/tested Oidc dialogs. CHUI regression tests are passed.

Did you also test Web ChUI?

Yes, Hotel, Hotel_GUI, the customers applications and testcases. Committed rev 16645 (10915a) removed commented code.

No more testing planned?

Yes, I tested rev 16644 (10915a) but as rev 16645 is only a cleanup, then it seems done.