Skip to content

chore(fe): polish Query History table#9767

Merged
jmelahman merged 2 commits intomainfrom
jamison/Query-history-table
Mar 30, 2026
Merged

chore(fe): polish Query History table#9767
jmelahman merged 2 commits intomainfrom
jamison/Query-history-table

Conversation

@jmelahman
Copy link
Copy Markdown
Contributor

@jmelahman jmelahman commented Mar 30, 2026

Description

Removes some custom HTML from this page

How Has This Been Tested?

before
20260330_12h20m50s_grim

after
20260330_12h20m55s_grim

Additional Options

  • [Optional] Please cherry-pick this PR to the latest release version.
  • [Optional] Override Linear Check

Summary by cubic

Polished the Query History table by standardizing layout with Section, improving export UX with a proper Button for downloads, and tightening table spacing and pagination.

  • Refactors
    • Used Section for filter controls, the main table block, and both paginations.
    • Simplified the Previous Exports modal by removing extra wrappers around the table.
    • Replaced the download link with a small, tertiary Button with icon; disabled until SUCCESS, with a tooltip and href only when ready.
    • Tweaked table spacing for more consistent layout.

Written for commit 631211c. Summary will update on new commits.

@jmelahman jmelahman requested a review from a team as a code owner March 30, 2026 19:21
@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps bot commented Mar 30, 2026

Greptile Summary

This PR polishes the Query History table by replacing ad-hoc HTML wrapper divs with the standardized Section layout component, simplifying the Previous Exports modal table structure, and upgrading the CSV download from a raw <a> tag to a Button with proper disabled state, tooltip feedback, and icon.

  • SelectFeedbackType: replaced outer <div> + inner <div className="max-w-sm space-y-6"> with <Section alignItems="start" gap={0.25}>, which also removes the no-longer-needed my-auto mr-2 classes on the label.
  • PreviousQueryHistoryExportsModal: removed two wrapper divs around the <Table>, making the table a direct child of Modal.Body.
  • Download cell: replaced conditional <a> / dimmed <SvgDownloadCloud> with a single <Button> that disables itself and shows a tooltip when the export is not yet ready.
  • Main QueryHistoryTable: table and pagination are now inside a <Section> instead of bare divs with one-off margin/flex utilities.
  • One minor observation: the <Section> wrapping <PageSelector> (lines 235–241 in the modal and 365–373 in the main table) is nested inside another <Section> that already centers its children, making the inner wrapper redundant.

Confidence Score: 5/5

  • Safe to merge — changes are purely presentational with no logic or data-flow impact.
  • All changes are UI-layer refactoring that replaces custom HTML wrappers with the existing Section component and improves the download UX. No business logic, API calls, or data transformations are touched. Before/after screenshots confirm the intended visual output. The only finding is a P2 style suggestion about a redundant nested Section for pagination.
  • No files require special attention.

Important Files Changed

Filename Overview
web/src/app/ee/admin/performance/query-history/QueryHistoryTable.tsx UI polish refactor: replaces raw HTML wrappers with Section layout component, upgrades download anchor to a Button with disabled state and tooltip, and removes redundant nesting in the exports modal table

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    QHT[QueryHistoryTable]
    CS[CardSection]
    F[Filters div\nSelectFeedbackType + DateRange]
    SEP[Separator]
    S1[Section]
    T[Table\nQueryHistoryTableRow ×N]
    S2[Section\nPageSelector]
    MOD[PreviousQueryHistoryExportsModal]
    MB[Modal.Body]
    ET[Table\nExport rows]
    S3[Section\nPageSelector]

    QHT --> CS
    CS --> F
    CS --> SEP
    CS --> S1
    S1 --> T
    S1 --> S2
    QHT --> MOD
    MOD --> MB
    MB --> ET
    MB --> S3

    style S1 fill:#d4edda,stroke:#28a745
    style S2 fill:#fff3cd,stroke:#ffc107
    style S3 fill:#fff3cd,stroke:#ffc107
Loading
Prompt To Fix All With AI
This is a comment left during a code review.
Path: web/src/app/ee/admin/performance/query-history/QueryHistoryTable.tsx
Line: 365-373

Comment:
**Redundant nested `<Section>` for pagination**

The inner `<Section>` wrapping `PageSelector` is redundant. The outer `<Section>` already defaults to `justifyContent="center"` and `alignItems="center"`, which will center the `PageSelector` just as well. The extra wrapper only adds another `h-full min-h-0 w-full` `div` to the DOM without contributing layout value.

The same pattern occurs at line 235–241 in the modal pagination.

```suggestion
          {chatSessionData && (
            <PageSelector
              totalPages={totalPages}
              currentPage={currentPage}
              onPageChange={goToPage}
            />
          )}
```

How can I resolve this? If you propose a fix, please make it concise.

Reviews (3): Last reviewed commit: "nit" | Re-trigger Greptile

Copy link
Copy Markdown
Contributor

@cubic-dev-ai cubic-dev-ai bot left a comment

Choose a reason for hiding this comment

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

2 issues found across 1 file

Confidence score: 4/5

  • Overall risk looks low; issues are minor and localized to QueryHistoryTable.tsx, with no indication of broad functional changes.
  • Most notable issue is a temporary console.log left in QueryHistoryTable.tsx, which could leak noise into production logs but is easy to fix.
  • Pay close attention to web/src/app/ee/admin/performance/query-history/QueryHistoryTable.tsx - remove debug logging and switch export row keys to a stable identifier.
Prompt for AI agents (unresolved issues)

Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.


<file name="web/src/app/ee/admin/performance/query-history/QueryHistoryTable.tsx">

<violation number="1" location="web/src/app/ee/admin/performance/query-history/QueryHistoryTable.tsx:199">
P2: Use a stable key (e.g., `task.taskId`) instead of the array index for export rows.</violation>

<violation number="2" location="web/src/app/ee/admin/performance/query-history/QueryHistoryTable.tsx:299">
P1: Custom agent: **No debugging code**

Remove the temporary `console.log` debug statement from `QueryHistoryTable` before merging.</violation>
</file>

Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.

@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Mar 30, 2026

Preview Deployment

Status Preview Commit Updated
https://onyx-preview-csx7fwto4-danswer.vercel.app 631211c 2026-03-30 20:55:57 UTC

Copy link
Copy Markdown
Contributor

@cubic-dev-ai cubic-dev-ai bot left a comment

Choose a reason for hiding this comment

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

No issues found across 1 file

Confidence score: 5/5

  • Automated review surfaced no issues in the provided summaries.
  • No files require special attention.

@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Mar 30, 2026

🖼️ Visual Regression Report

Project Changed Added Removed Unchanged Report
admin 7 0 0 157 View Report
exclusive 0 0 0 8 ✅ No changes

@jmelahman jmelahman force-pushed the jamison/Query-history-table branch from 34d0d13 to 631211c Compare March 30, 2026 20:52
@jmelahman jmelahman added this pull request to the merge queue Mar 30, 2026
Merged via the queue into main with commit 0123133 Mar 30, 2026
69 of 71 checks passed
@jmelahman jmelahman deleted the jamison/Query-history-table branch March 30, 2026 21:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants