Adding ability to load successfully processed messages using the common MessageView component #2311

johnsimons · 2025-04-04T06:45:40Z

This PR ensures the MessageView is reused for both successful and failure messages

johnsimons · 2025-04-04T07:27:16Z

src/Frontend/src/resources/Message.ts

  body_size: number;
  instance_id: string;
 }
-export interface ExtendedMessage extends Message {


It doesn't seem to be needed

johnsimons · 2025-04-04T07:27:37Z

src/Frontend/src/composables/toast.ts

  });
 }
+
+export const showToastAfterOperation = async (operation: () => Promise<void>, toastType: TYPE, title: string, message: string) => {


a helper function

johnsimons · 2025-04-04T07:29:32Z

src/Frontend/src/router/routeLinks.ts

 const messagesLinks = (root: string) => {
  return {
    root,
-    message: { link: (id: string) => `${root}/${id}`, template: "/messages/:id" },


The way success messages vs failed messages are loaded is different hence the 2 different options

More changes

Introduces a reusable `MetadataLabel` component to streamline the display of message metadata in the message view. This improves code readability and maintainability by encapsulating the common styling and tooltip logic for metadata labels. It also makes it easier to consistently apply styling across different metadata labels.

Replaces the custom tab navigation in the message view with a reusable TabsLayout component. This change improves code maintainability and reduces duplication by centralizing the tab layout logic. The tabs are now dynamically generated based on the message context, such as whether it's an error or if MassTransit is connected.

More

Copilot

Copilot reviewed 27 out of 27 changed files in this pull request and generated 1 comment.

src/Frontend/src/components/messages2/DeleteMessageButton.vue

Co-authored-by: Copilot <[email protected]>

PhilBastian

review WIP, leaving current comments

PhilBastian · 2025-04-08T07:37:49Z

src/Frontend/src/components/failedmessages/EditRetryDialog2.vue

+    headers: headers.value.data.map((header: Header) => ({ ...header })) as HeaderWithEditing[],
+    messageBody: body.value.data.value ?? "",
+  };
+  localMessage.value.isBodyEmpty = false;


already set above

PhilBastian · 2025-04-08T07:40:17Z

src/Frontend/src/components/failedmessages/EditRetryDialog2.vue

+
+function initializeMessageBodyAndHeaders() {
+  origMessageBody = body.value.data.value ?? "";
+  localMessage.value = {


It's likely not affecting anything right now, but anything that's watching localMessage will be triggered by this assignment, but not by subsequent changes to the object below. This should be a local const assignment, and then at the end of the function is where the localMessage.value gets set. Subtle bug otherwise for possible future functionality

I thought the trigger will only happen at the end of the function or async/await.
and the value on the watch callback will still be the end result.
but I can change it.

since it's a proxy it happens on any property change

PhilBastian · 2025-04-08T07:41:47Z

src/Frontend/src/components/failedmessages/EditRetryDialog2.vue

+
+function togglePanel(panelNum: number) {
+  panel.value = panelNum;
+  return false;


shouldn't be required since .prevent is used

PhilBastian · 2025-04-08T07:42:56Z

src/Frontend/src/components/failedmessages/EditRetryDialog2.vue

+                          </tr>
+                        </tbody>
+                      </table>
+                      <div role="tabpanel" v-if="panel === 2 && !localMessage.bodyUnavailable" style="height: calc(100% - 260px)">


local styles are better handled in styles if they don't involve any dynamic values

PhilBastian · 2025-04-08T07:58:35Z

src/Frontend/src/components/failedmessages/EditRetryDialog2.vue

+                          <CodeEditor v-else aria-label="message body" :read-only="!localMessage.isContentTypeSupported" v-model="localMessage.messageBody" :language="localMessage.language" :show-gutter="true"></CodeEditor>
+                        </div>
+                        <span class="empty-error" v-if="localMessage.isBodyEmpty"><i class="fa fa-exclamation-triangle"></i> Message body cannot be empty</span>
+                        <span class="reset-body" v-if="localMessage.isBodyChanged"><i class="fa fa-undo" v-tippy="`Reset changes`"></i> <a @click="resetBodyChanges()" href="javascript:void(0)">Reset changes</a></span>


while we're re-creating this control, we really should replace these anchor elements with button elements, fix the styling and get rid of the javascript:void(0)

I’ll raise a card for this

PhilBastian · 2025-04-08T07:59:25Z

src/Frontend/src/components/failedmessages/MessageList.vue


 function navigateToMessage(messageId: string) {
-  router.push({ path: routeLinks.messages.message.link(messageId), query: { back: route.path } });
+  router.push({ path: routeLinks.messages.failedMessage.link(messageId) });


why don't we need back anymore?

Well, there are occasions where we navigate to the same page just with a different id.
So things started to get complex and all we are doing at the end of the day is a manual attempt at replacing the back button in the browser.
So I started questioning the complexity vs the benefit.
What are your thoughts?

in the cases where they're navigating to multiple messages without changing view, I would think they would still want an easy way to get back to the list that originally brought them to the messageview, so this back value should be retained through multiple messageview id changes

I have raised a card to add the back button back

src/Frontend/src/components/messages2/BodyView.vue

src/Frontend/src/components/messages2/DeleteMessageButton.vue

PhilBastian · 2025-04-08T09:02:00Z

src/Frontend/src/stores/MessageViewStore.ts

@@ -0,0 +1,314 @@
+import { acceptHMRUpdate, defineStore } from "pinia";


According to how all the other stores are named, this should be MessageStore and useMessageStore rather than MessageViewStore

Co-authored-by: Phil Bastian <[email protected]>

PhilBastian · 2025-04-08T23:01:19Z

src/Frontend/src/stores/MessageViewStore.ts

+      }
+
+      const message = (await response.json()) as FailedMessage;
+      state.data.message_id = message.message_id;


this feels like it belongs in a "modelFromMessage" function

PhilBastian · 2025-04-08T23:02:25Z

src/Frontend/src/stores/MessageViewStore.ts

+  }
+
+  async function loadFailedMessage(id: string) {
+    state.data.id = id;


this is done as part of the loadMessage - probably only belongs there

PhilBastian · 2025-04-08T23:05:40Z

src/Frontend/src/stores/MessageViewStore.ts

+  async function archiveMessage() {
+    if (state.data.id) {
+      await useArchiveMessage([state.data.id]);
+      state.data.failure_status.archiving = true;


where does this get set back to false?

when it gets reloaded as part of the pollForNextUpdate

PhilBastian · 2025-04-08T23:07:31Z

src/Frontend/src/components/failedmessages/EditRetryDialog2.vue

+
+function initializeMessageBodyAndHeaders() {
+  origMessageBody = body.value.data.value ?? "";
+  localMessage.value = {


since it's a proxy it happens on any property change

PhilBastian · 2025-04-08T23:09:38Z

src/Frontend/src/components/failedmessages/MessageList.vue


 function navigateToMessage(messageId: string) {
-  router.push({ path: routeLinks.messages.message.link(messageId), query: { back: route.path } });
+  router.push({ path: routeLinks.messages.failedMessage.link(messageId) });


in the cases where they're navigating to multiple messages without changing view, I would think they would still want an easy way to get back to the list that originally brought them to the messageview, so this back value should be retained through multiple messageview id changes

johnsimons commented Apr 4, 2025

View reviewed changes

johnsimons changed the title ~~Introducing the messageview store~~ Adding ability to load successfully processed messages using the common MessageView component Apr 4, 2025

johnsimons added 12 commits April 4, 2025 17:33

Introducing the messageview store

02f37c6

Modifying view to use store

14111b2

More changes

Changing to use storeToRefs

00de7e5

A few small fixes

f0d8d2e

Adding auto refresh

18a471f

More

No longer needed

2b48a4d

Clean up

865c1c7

Remove extension

aa1a7dd

Bring back the previous version to be backwards compat

b737536

remove flag

506a349

johnsimons force-pushed the john/common_message_view branch from bfd1fa8 to 506a349 Compare April 4, 2025 07:33

johnsimons marked this pull request as ready for review April 7, 2025 00:05

johnsimons requested review from PhilBastian, cquirosj and Copilot April 7, 2025 00:06

Copilot AI reviewed Apr 7, 2025

View reviewed changes

src/Frontend/src/components/messages2/DeleteMessageButton.vue Outdated Show resolved Hide resolved

johnsimons self-assigned this Apr 7, 2025

johnsimons and others added 4 commits April 7, 2025 11:27

Fixed formatting

bb152f3

Co-authored-by: Copilot <[email protected]>

Merge branch 'master' into john/common_message_view

41fbae9

Fix formatting issue

26219fb

Merge branch 'master' into john/common_message_view

0a77661

PhilBastian reviewed Apr 8, 2025

View reviewed changes

Apply suggestions from code review

0839e11

Co-authored-by: Phil Bastian <[email protected]>

PhilBastian approved these changes Apr 8, 2025

View reviewed changes

A few changes based on feedback

4f6a6a7

johnsimons enabled auto-merge April 9, 2025 06:06

johnsimons merged commit 95fd4a4 into master Apr 9, 2025
5 checks passed

johnsimons deleted the john/common_message_view branch April 9, 2025 06:11

		@@ -0,0 +1,314 @@
		import { acceptHMRUpdate, defineStore } from "pinia";

Adding ability to load successfully processed messages using the common MessageView component #2311

Adding ability to load successfully processed messages using the common MessageView component #2311

Uh oh!

Conversation

johnsimons commented Apr 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

PhilBastian left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

johnsimons commented Apr 4, 2025 •

edited

Loading