feat(webui): Minor UI improvements: by davemarco · Pull Request #1511 · y-scope/clp

davemarco · 2025-10-27T21:37:49Z

Description

Prevent stale result metadata from updating state.
Recalculate result table height when switching sql interface.
Prevent calling toFixed on null value in query speed status.
Reduce size of results table to remove scroll bar

PR #1482 made changes to result metadata that resulted in stale result metadata overwriting query state. Issue occurred during lint/review and could not retest since presto was down. This PR reverts this issue. Also I noticed the table height was not resizing properly when changing interfaces. This PR fixes as well. Lastly, I noticed an error showing up when calling a function in query speed. I believe the function is being called before the value is populated, so added a ternary to fix.

Checklist

The PR satisfies the contribution guidelines.
This is a breaking change and that has been indicated in the PR title, OR this isn't a
breaking change.
Necessary docs have been updated, OR no docs need to be updated.

Validation performed

Tested presto and CLP-s UI

Summary by CodeRabbit

Bug Fixes
- Graceful fallback when search speed data is incomplete to avoid incorrect metrics.
- Search results table now recalculates height when the query context changes.
UI
- Increased bottom padding of the search results table for improved spacing.
Refactor
- Internal search state handling updated for more reliable, less stale updates.

coderabbitai · 2025-10-27T21:37:59Z

Walkthrough

Added null-safe handling for query speed values and widened the QuerySpeed response type to permit nulls; refactored a state-update hook to read current UI state inside an effect; and made SearchResultsTable recompute layout when sqlInterface changes.

Changes

Cohort / File(s)	Summary
QuerySpeed types & runtime null-safety `components/webui/client/src/pages/SearchPage/SearchControls/QueryStatus/QuerySpeed/utils.ts`, `components/webui/client/src/pages/SearchPage/SearchControls/QueryStatus/QuerySpeed/index.tsx`	`QuerySpeedResp` now allows `bytes: number
SearchResultsTable effect dependency `components/webui/client/src/pages/SearchPage/SearchResults/SearchResultsTable/index.tsx`	Imported `usePrestoSearchState`, extracted `sqlInterface`, and added `sqlInterface` to the height-calculation `useEffect` dependency array so the effect re-runs when `sqlInterface` changes.
UseUpdateStateWithMetadata effect refactor `components/webui/client/src/pages/SearchPage/SearchState/useUpdateStateWithMetadata.ts`	Removed destructured `searchUiState` capture; inside the effect the code now reads current `searchUiState` via `useSearchStore.getState()` and `searchUiState` was removed from the effect dependencies to avoid stale closure use.
Layout constant tweak `components/webui/client/src/pages/SearchPage/SearchResults/SearchResultsTable/typings.ts`	`TABLE_BOTTOM_PADDING` constant updated from `75` to `95`.

Sequence Diagram(s)

sequenceDiagram
  autonumber
  actor User
  participant UI as SearchResultsTable/UI
  participant Presto as PrestoState (`sqlInterface`)
  participant Effect as HeightCalcEffect
  Note over Presto,Effect #ddeeff: When sqlInterface changes
  Presto ->> UI: sqlInterface updated
  UI ->> Effect: effect re-runs (depends on sqlInterface)
  Effect ->> UI: recompute height/layout

sequenceDiagram
  autonumber
  participant Component as QuerySpeed component
  participant Utils as fetchQuerySpeed
  participant Const as SPEED_DEFAULT
  Component ->> Utils: fetchQuerySpeed()
  Utils -->> Component: { bytes: number|null, duration: number|null }
  alt bytes or duration is null
    Component ->> Const: return SPEED_DEFAULT
  else both present
    Component ->> Component: compute latency/speed
  end

sequenceDiagram
  autonumber
  participant EffectFile as useUpdateStateWithMetadata effect
  participant Store as useSearchStore
  EffectFile ->> Store: useSearchStore.getState() (inside effect)
  Store -->> EffectFile: current searchUiState
  alt guard passes
    EffectFile ->> EffectFile: proceed with metadata update
  else guard fails
    EffectFile ->> EffectFile: early return
  end

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Pay attention to callers of QuerySpeedResp for proper null handling.
Verify index.tsx display logic yields SPEED_DEFAULT when expected.
Check SearchResultsTable effect for extra re-renders or layout thrashing.
Confirm useUpdateStateWithMetadata change does not introduce race conditions or missed updates.

Pre-merge checks and finishing touches

❌ Failed checks (1 warning, 1 inconclusive)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.	You can run `@coderabbitai generate docstrings` to improve docstring coverage.
Title Check	❓ Inconclusive	The pull request title "feat(webui): Minor UI improvements:" is overly vague and generic. While it is technically related to the changeset (the modifications do make UI-related improvements), the title uses a non-descriptive phrase that fails to convey the specific nature of the changes. The PR actually contains three distinct technical fixes: preventing stale metadata from updating query state, recalculating table height when switching SQL interfaces, and adding null checks to prevent errors in query speed calculations. A developer scanning commit history would not understand these primary objectives from the current title alone.	Consider revising the title to be more specific and descriptive, such as "fix(webui): Fix stale metadata state and SQL interface switching" or another title that explicitly references the main issues being addressed. This would help maintainers and contributors quickly understand the purpose of the changes when reviewing history.

✅ Passed checks (1 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.

✨ Finishing touches

📝 Generate docstrings

🧪 Generate unit tests (beta)

Create PR with unit tests
Post copyable unit tests in a comment

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

coderabbitai

Actionable comments posted: 1

📜 Review details

Configuration used: CodeRabbit UI

Review profile: ASSERTIVE

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 69fbe45 and f57f212.

📒 Files selected for processing (3)

components/webui/client/src/pages/SearchPage/SearchControls/QueryStatus/QuerySpeed/index.tsx (1 hunks)
components/webui/client/src/pages/SearchPage/SearchResults/SearchResultsTable/index.tsx (3 hunks)
components/webui/client/src/pages/SearchPage/SearchState/useUpdateStateWithMetadata.ts (1 hunks)

🧰 Additional context used

📓 Path-based instructions (1)

**/*.{cpp,hpp,java,js,jsx,tpp,ts,tsx}

⚙️ CodeRabbit configuration file

Prefer false == <expression> rather than !<expression>.

Files:

components/webui/client/src/pages/SearchPage/SearchResults/SearchResultsTable/index.tsx
components/webui/client/src/pages/SearchPage/SearchState/useUpdateStateWithMetadata.ts
components/webui/client/src/pages/SearchPage/SearchControls/QueryStatus/QuerySpeed/index.tsx

🪛 GitHub Actions: clp-lint

components/webui/client/src/pages/SearchPage/SearchControls/QueryStatus/QuerySpeed/index.tsx

[error] 42-42: Unnecessary optional chain on a non-nullish value. @typescript-eslint/no-unnecessary-condition

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)

GitHub Check: conventional-commits
GitHub Check: package-image

🔇 Additional comments (2)

components/webui/client/src/pages/SearchPage/SearchResults/SearchResultsTable/index.tsx (1)

11-11: LGTM! Table height now recalculates when SQL interface changes.

The import of usePrestoSearchState, extraction of sqlInterface, and addition to the effect dependency array correctly implements the stated objective. When the SQL interface switches, the effect will re-run, recalculating the table height based on current DOM dimensions.

Also applies to: 25-25, 44-44

components/webui/client/src/pages/SearchPage/SearchState/useUpdateStateWithMetadata.ts (1)

41-45: LGTM! Reading state inside effect prevents stale closure issue.

Reading searchUiState via useSearchStore.getState() inside the effect ensures the current state is checked rather than a stale closure value from render time. This correctly prevents stale result metadata from updating query state, addressing the issue introduced by PR #1482 as noted in the PR description.

hoophalab

sorry, but there is still a scroll bar. Is this an issue specific to my machine?

davemarco · 2025-10-28T14:48:55Z

I adjusted the table to not grow as large should fix. My mac hides the scroll bar so didn't see.

junhaoliao

deferring to @hoophalab 's review

- Prevent stale result metadata from updating state. - Recalculate result table height when switching sql interface. - Prevent calling toFixed on null value in query speed status. - Reduce size of results table to remove scroll bar.

marco added 2 commits October 27, 2025 21:16

latest

76ce316

latest

f57f212

davemarco requested a review from hoophalab October 27, 2025 21:38

davemarco marked this pull request as ready for review October 27, 2025 21:38

davemarco requested a review from a team as a code owner October 27, 2025 21:38

coderabbitai Bot reviewed Oct 27, 2025

View reviewed changes

Comment thread components/webui/client/src/pages/SearchPage/SearchControls/QueryStatus/QuerySpeed/index.tsx Outdated

latest

728b1e1

hoophalab reviewed Oct 28, 2025

View reviewed changes

latest

46cffc4

davemarco requested a review from hoophalab October 28, 2025 14:49

davemarco changed the title ~~fix(webui): Minor UI improvements:~~ feat(webui): Minor UI improvements: Oct 28, 2025

Merge branch 'main' into fix4

1f5eeef

hoophalab approved these changes Oct 28, 2025

View reviewed changes

hoophalab requested a review from junhaoliao October 28, 2025 15:30

Merge branch 'main' into fix4

4f85bd2

junhaoliao approved these changes Oct 28, 2025

View reviewed changes

Merge branch 'main' into fix4

eb1c23f

davemarco merged commit cf28e78 into y-scope:main Oct 28, 2025
19 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(webui): Minor UI improvements:#1511

feat(webui): Minor UI improvements:#1511
davemarco merged 7 commits into
y-scope:mainfrom
davemarco:fix4

davemarco commented Oct 27, 2025 •

edited

Loading

Uh oh!

coderabbitai Bot commented Oct 27, 2025 •

edited

Loading

Uh oh!

coderabbitai Bot left a comment

Uh oh!

Uh oh!

hoophalab left a comment •

edited

Loading

Uh oh!

davemarco commented Oct 28, 2025 •

edited

Loading

Uh oh!

junhaoliao left a comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

davemarco commented Oct 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Checklist

Validation performed

Summary by CodeRabbit

Uh oh!

coderabbitai Bot commented Oct 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

Sequence Diagram(s)

Estimated code review effort

Pre-merge checks and finishing touches

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

hoophalab left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

davemarco commented Oct 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

junhaoliao left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

davemarco commented Oct 27, 2025 •

edited

Loading

coderabbitai Bot commented Oct 27, 2025 •

edited

Loading

hoophalab left a comment •

edited

Loading

davemarco commented Oct 28, 2025 •

edited

Loading