Skip to content

fix: Execution details and display sources on the debug conversation page should remain visible at all times. #7810

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
shaohuzhang1 wants to merge 2 commits into from
Closed

fix: Execution details and display sources on the debug conversation page should remain visible at all times. #7810

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
1d221b1
fix: [Portal] If you sign out of the portal while logged in and using…
shaohuzhang1 Nov 10, 2026
83b3341
fix: Execution details and display sources on the debug conversation …
shaohuzhang1 Nov 10, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion apps/application/views/application_chat.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -346,7 +346,7 @@ def get(
"chat_user_id": str(request.user.id),
}
)
return result.success(serializer.page(current_page, page_size))
return result.success(serializer.debug_page(current_page, page_size))

class Operate(APIView):
authentication_classes = [TokenAuth]

@shaohuzhang1 shaohuzhang1 Nov 10, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changing method names from page to debug_page introduces several risks and poor practices:

  1. Name Ambiguity: "Debug" implies functionality for troubleshooting, not standard pagination. This is misleading for API consumers and maintainers.
  2. Breaking Change: If serializer.page() was part of the public or internal contract, renaming it to debug_page() breaks any other code or tests relying on the original method name on this serializer.
  3. Misuse of Production Code: Debug-specific logic or naming should generally be abstracted away (via settings flags, separate utility functions, or environment checks) rather than hardcoded into production API responses. It clutters the domain logic.

Recommendation:
Revert to result.success(serializer.page(current_page, page_size)).
If you intentionally need debug-only behavior (e.g., returning raw data or extra fields), implement it via a configuration flag or separate endpoint, not by altering the core pagination method's name in production code paths.

Expand Down
9 changes: 9 additions & 0 deletions apps/chat/serializers/chat_record.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -214,6 +214,15 @@ def list(self):
queryset = self.get_queryset()
return [ChatRecordSerializerModel(r).data for r in queryset]

def debug_page(self, current_page, page_size):
self.is_valid(raise_exception=True)
application_access_token = (
QuerySet(ApplicationAccessToken).filter(application_id=self.data.get("application_id")).first()
)
return ApplicationChatRecordQuerySerializers(
data={"application_id": self.data.get("application_id"), "chat_id": self.data.get("chat_id")}
).page(current_page, page_size, show_source=True, show_exec=True)

def page(self, current_page, page_size):
self.is_valid(raise_exception=True)
application_access_token = (

@shaohuzhang1 shaohuzhang1 Nov 10, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here is an analysis of the added debug_page method:

🚨 Critical Issues

  1. Unused Variable (Security/Logic Flaw)

    • application_access_token is fetched but never used.
    • Implication: If this token was intended to authorize or filter results, the debug endpoint currently leaks data without proper validation. If it wasn't needed for the logic, it’s dead code and should be removed to avoid confusion and unnecessary DB queries.

  2. Inconsistent Validation

    • The original list() method does not call self.is_valid(raise_exception=True), but both debug_page() and the existing page() method do. This inconsistency might lead to unexpected behavior depending on how self (the serializer/request payload) is populated. Ensure all entry points consistently validate input.

  3. Hardcoded Debug Flag

    • show_source=True, show_exec=True are hardcoded. This exposes internal execution details/source code to the client. Ensure this endpoint is strictly gated by admin/debug permissions in your URL routing or view middleware, as this is a significant information disclosure risk in production.

💡 Optimization & Style Suggestions

  1. Redundant Data Extraction

    • You extract application_id and chat_id from self.data multiple times. Store them in variables for readability and slight performance gain:
    app_id = self.data.get("application_id")
chat_id = self.data.get("chat_id")

  2. QuerySet Usage

    • QuerySet(ApplicationAccessToken).filter(...) is inefficient. Use the model directly:
      ApplicationAccessToken.objects.filter(application_id=app_id).first()

  3. Serializer Instantiation

    • If ApplicationChatRecordQuerySerializers is just a wrapper for pagination, ensure that instantiating it with only data (without instance) is correct for your library (e.g., DRF). Usually, paginated queries are done on the queryset itself, not via a secondary serializer instantiation unless specifically designed for that pattern.

✅ Recommended Refactored Code

    def debug_page(self, current_page, page_size):
        self.is_valid(raise_exception=True)
        
        app_id = self.data.get("application_id")
        chat_id = self.data.get("chat_id")

        # Option A: Remove if token check isn't enforced here
        # Option B: Add check if token is required:
        # if not ApplicationAccessToken.objects.filter(application_id=app_id).exists():
        #     raise PermissionError("Invalid application")

        return ApplicationChatRecordQuerySerializers(
            data={"application_id": app_id, "chat_id": chat_id}
        ).page(current_page, page_size, show_source=True, show_exec=True)

🔐 Security Reminder

Ensure the route exposing debug_page is strictly restricted to authorized developers/admins using authentication middleware or permission classes (e.g., IsAdminRole, DEBUG=True env checks), as it exposes low-level execution data.

Expand Down
10 changes: 8 additions & 2 deletions ui/src/conversation-panel/left-sidebar/account-menu/index.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,11 +28,17 @@ const tokenPercentage = computed(() => {
const userPwdDialogRef = useTemplateRef<InstanceType<typeof UserPwdDialog>>('userPwdDialogRef')
const apiKeyDialogRef = useTemplateRef<InstanceType<typeof ApiKeyDialog>>('apiKeyDialogRef')

// 退出对话账号后重新导航，由守卫处理后续认证。
// 退出对话账号后回到当前场景首页，不停留在上一用户的对话；由守卫决定匿名认证或跳转登录页。
const handleLogout = () => {
return auth
.logout()
.then(() => router.replace({ path: router.currentRoute.value.fullPath, force: true }))
.then(() =>
router.replace(
route.meta.portal
? { name: 'portal-application-list', force: true }
: { name: 'chat-home', params: { accessToken: route.params.accessToken }, force: true },
),
)
.catch(() => {})
}

@shaohuzhang1 shaohuzhang1 Nov 10, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here is a code review for the provided snippet.

Critical Issues

  1. force: true placement: The force: true parameter is part of the NavigationGuardNext or router.push/replace arguments, but it must be a top-level property alongside name, path, or params. In your object literals, it is correctly placed at the same level as name and params, so this is technically valid Vue Router syntax, but it's worth noting that force is often used to bypass identical route checks. Ensure this is intended.
  2. Potential undefined access: route.params.accessToken assumes accessToken exists in the params. If the current route does not have this param, undefined will be passed. If chat-home requires this param, navigation might fail or behave unexpectedly. Consider optional chaining route.params.accessToken (if Vue Router allows undefined in params) or defaulting to a safe value if the param is missing.

Optimization & Suggestions

  1. Code Clarity: The ternary operator inside router.replace() is slightly hard to read. It can be extracted for better readability.
  2. Error Handling: .catch(() => {}) silently swallows errors. At minimum, log the error to debugging tools to avoid hidden failures.
  3. Type Safety: useTemplateRef is fine, but ensure UserPwdDialog and ApiKeyDialog types are correctly imported.

Refactored Example

const handleLogout = async () => {
  try {
    await auth.logout();
    
    // Determine target route based on current context
    const targetRoute = route.meta.portal
      ? { name: 'portal-application-list', force: true }
      : { 
          name: 'chat-home', 
          params: { accessToken: route.params.accessToken }, 
          force: true 
        };

    await router.replace(targetRoute);
  } catch (error) {
    console.error('Logout failed:', error);
  }
};

Summary of Changes

  • Used async/await for cleaner asynchronous flow.
  • Extracted route definition into a variable for better readability.
  • Added meaningful error logging instead of silently catching.
  • Kept logic identical but improved maintainability.

Expand Down
Loading