feat(webui): Make max search results configurable via Helm#2246
feat(webui): Make max search results configurable via Helm#2246goynam wants to merge 1 commit intoy-scope:mainfrom
Conversation
WalkthroughAdds a configurable search max-results limit: exposes ChangesSearch Max Results Configuration
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tools/deployment/package-helm/templates/webui-deployment.yaml`:
- Around line 58-59: The YAML linter flags the Helm template expression on the
SEARCH_MAX_NUM_RESULTS line for the "braces" rule; suppress it by adding an
inline yamllint disable comment for the braces rule on that line in
tools/deployment/package-helm/templates/webui-deployment.yaml (e.g., add a "#
yamllint disable-line braces" style comment right after or on the same line as
the SEARCH_MAX_NUM_RESULTS mapping) so the diff-scoped linter won't fail while
preserving the existing quoted Helm template expression.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 06efef7b-5726-44af-a977-25b87ffe04b3
📒 Files selected for processing (5)
components/webui/server/src/plugins/external/env.tscomponents/webui/server/src/routes/api/search/index.tscomponents/webui/server/src/routes/api/search/typings.tstools/deployment/package-helm/templates/webui-deployment.yamltools/deployment/package-helm/values.yaml
goynam
force-pushed
the
feat/configurable-webui-max-results
branch
from
b42745f to
c52cca1
Compare
The maximum number of search results returned by the WebUI was hardcoded to 1000. This makes it configurable via: - Environment variable: SEARCH_MAX_NUM_RESULTS - Helm values: clpConfig.webui.max_num_results The default remains 1000 for backward compatibility.
goynam
force-pushed
the
feat/configurable-webui-max-results
branch
from
c52cca1 to
e7f85dc
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@components/webui/server/src/routes/api/search/index.ts`:
- Line 19: The import statement bringing in SEARCH_MAX_NUM_RESULTS and
setSearchMaxNumResults from "./typings.js" should be split across multiple lines
to satisfy import-newlines/enforce and `@stylistic/object-curly-newline`; update
the import for the module "./typings.js" so the named specifiers
(SEARCH_MAX_NUM_RESULTS and setSearchMaxNumResults) each appear on their own
line (or otherwise broken across lines) inside the braces while preserving the
same identifiers and relative order.
In `@components/webui/server/src/routes/api/search/typings.ts`:
- Around line 16-24: Replace the magic numeric literal by introducing a named
constant and use it to initialize the mutable variable: add a const like
DEFAULT_SEARCH_MAX_NUM_RESULTS (value 1000) and change the mutable
SEARCH_MAX_NUM_RESULTS initialization to use that constant; keep the setter
setSearchMaxNumResults as-is so the runtime mutability remains but the literal
is no longer inline.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: d3762c92-35f9-49a6-987f-c940371f7f4f
📒 Files selected for processing (6)
components/webui/server/src/plugins/external/env.tscomponents/webui/server/src/routes/api/search/index.tscomponents/webui/server/src/routes/api/search/typings.tstools/deployment/package-helm/Chart.yamltools/deployment/package-helm/templates/webui-deployment.yamltools/deployment/package-helm/values.yaml
|
|
||
| import settings from "../../../../settings.json" with {type: "json"}; | ||
| import {SEARCH_MAX_NUM_RESULTS} from "./typings.js"; | ||
| import {SEARCH_MAX_NUM_RESULTS, setSearchMaxNumResults} from "./typings.js"; |
There was a problem hiding this comment.
Split the named import across lines to satisfy the active lint rules.
Line 19 fails import-newlines/enforce and @stylistic/object-curly-newline, which blocks the lint job.
Proposed fix
-import {SEARCH_MAX_NUM_RESULTS, setSearchMaxNumResults} from "./typings.js";
+import {
+ SEARCH_MAX_NUM_RESULTS,
+ setSearchMaxNumResults,
+} from "./typings.js";📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| import {SEARCH_MAX_NUM_RESULTS, setSearchMaxNumResults} from "./typings.js"; | |
| import { | |
| SEARCH_MAX_NUM_RESULTS, | |
| setSearchMaxNumResults, | |
| } from "./typings.js"; |
🧰 Tools
🪛 GitHub Actions: clp-lint
[error] 19-19: ESLint import-newlines/enforce: Imports must be broken into multiple lines if there are more than 1 elements.
[error] 19-19: ESLint @stylistic/object-curly-newline: Expected a line break after this opening brace.
[error] 19-19: ESLint @stylistic/object-curly-newline: Expected a line break before this closing brace.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@components/webui/server/src/routes/api/search/index.ts` at line 19, The
import statement bringing in SEARCH_MAX_NUM_RESULTS and setSearchMaxNumResults
from "./typings.js" should be split across multiple lines to satisfy
import-newlines/enforce and `@stylistic/object-curly-newline`; update the import
for the module "./typings.js" so the named specifiers (SEARCH_MAX_NUM_RESULTS
and setSearchMaxNumResults) each appear on their own line (or otherwise broken
across lines) inside the braces while preserving the same identifiers and
relative order.
| let SEARCH_MAX_NUM_RESULTS = 1000; | ||
|
|
||
| /** | ||
| * Sets the max number of search results from the fastify config. | ||
| * @param maxResults | ||
| */ | ||
| const setSearchMaxNumResults = (maxResults: number) => { | ||
| SEARCH_MAX_NUM_RESULTS = maxResults; | ||
| }; |
There was a problem hiding this comment.
Fix the CI-blocking no-magic-numbers lint error in the mutable search limit.
Line 16 currently initializes a let with a numeric literal, which violates the configured ESLint rule and fails clp-lint.
Proposed fix
+const DEFAULT_SEARCH_MAX_NUM_RESULTS = 1_000;
+
/**
* The maximum number of results to retrieve for a search.
* Configured via the SEARCH_MAX_NUM_RESULTS environment variable (default: 1000).
*/
-let SEARCH_MAX_NUM_RESULTS = 1000;
+let SEARCH_MAX_NUM_RESULTS = DEFAULT_SEARCH_MAX_NUM_RESULTS;🧰 Tools
🪛 GitHub Actions: clp-lint
[error] 16-16: ESLint no-magic-numbers: Number constants declarations must use 'const'.
[warning] 19-19: ESLint jsdoc/tag-lines: Expected 1 lines after block description.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@components/webui/server/src/routes/api/search/typings.ts` around lines 16 -
24, Replace the magic numeric literal by introducing a named constant and use it
to initialize the mutable variable: add a const like
DEFAULT_SEARCH_MAX_NUM_RESULTS (value 1000) and change the mutable
SEARCH_MAX_NUM_RESULTS initialization to use that constant; keep the setter
setSearchMaxNumResults as-is so the runtime mutability remains but the literal
is no longer inline.
|
Thanks for working on this! To give some context: the max result count isn't really a proper mechanism for scalability - we'll likely drop it entirely in the future - but in the meantime, making it configurable is the right step, and it's great to see someone take it on. I've been thinking through the same problem and wanted to share some design considerations for the approach. The core difference is around how the configuration flows through the system and how complete the coverage is. 1. Configuration mechanism:
|
| Step | File | Change |
|---|---|---|
| Config definition | clp_config.py |
Add max_search_results: PositiveInt = 1000 to WebUi |
| Templates | client/public/settings.json, server/settings.json |
Add "MaxSearchResults": 1000 |
| Propagation | controller.py |
Add to both client & server settings_json_updates |
| Client types | client/src/settings.ts |
Add MaxSearchResults: number to Settings |
| Client config | client/src/config/index.ts |
Export SETTINGS_MAX_SEARCH_RESULTS |
| Client usage (3 sites) | typings.ts, useSearchResults.ts, usePrestoSearchResults.ts |
Replace hardcoded constant with SETTINGS_MAX_SEARCH_RESULTS |
| Presto guided | presto-guided-search-requests.ts |
Replace DEFAULT_SEARCH_LIMIT with SETTINGS_MAX_SEARCH_RESULTS |
| Server usage (3 sites) | search/typings.ts, search/index.ts, search/utils.ts, presto-search/typings.ts, presto-search/index.ts |
Use settings.MaxSearchResults instead of hardcoded/mutable constants |
| Package template | clp-config.template.json.yaml |
Add commented max_search_results: 1000 |
| Helm | values.yaml, configmap.yaml |
Add max_search_results to webui section and both settings JSON blocks |
This covers all search paths end-to-end, uses the existing config flow, and avoids introducing mutable global state.
Happy to help if you'd like to iterate on any of this!
2 participants
The maximum number of search results returned by the WebUI was hardcoded to 1000. This makes it configurable via:
The default remains 1000 for backward compatibility.
Description
Checklist
breaking change.
Validation performed
Summary by CodeRabbit
New Features
Chores