fix: redesign global search with cursor feeds#1945
Merged
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughIntroduces cursor-based global search functionality for DAGs and documents across the stack, including new REST API endpoints with pagination, core cursor encoding/decoding utilities, windowed grep matching, storage implementations with search methods, and a refactored UI with infinite-scroll feeds and scope-based filtering. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client / UI
participant Handler as Search Handler
participant Storage as DAG/Doc Storage
participant Grep as Grep Engine
Client->>Handler: GET /search/dags?q=...&cursor=...&limit=...
activate Handler
Handler->>Handler: Validate query parameter
Handler->>Storage: SearchCursor(ctx, opts)
activate Storage
Storage->>Storage: Decode cursor (version, query, offset)
Storage->>Storage: Walk directory, filter files
loop For each file up to limit
Storage->>Grep: GrepWindow(data, pattern, offset, limit)
activate Grep
Grep->>Grep: Scan content, track matches
Grep-->>Storage: WindowResult{matches, hasMore, nextOffset}
deactivate Grep
Storage->>Storage: Accumulate results
end
Storage-->>Handler: CursorResult{items, hasMore, nextCursor}, errors
deactivate Storage
Handler->>Handler: Convert to API response
Handler->>Handler: Encode nextCursor (page offset)
Handler-->>Client: DAGSearchFeedResponse{items, hasMore, nextCursor}
deactivate Handler
Client->>Handler: GET /search/dags/{fileName}/matches?q=...&cursor=...
activate Handler
Handler->>Storage: SearchMatches(ctx, fileName, opts)
activate Storage
Storage->>Grep: GrepWindow(data, pattern, offset, limit)
activate Grep
Grep-->>Storage: WindowResult{matches, hasMore, nextOffset}
deactivate Grep
Storage-->>Handler: CursorResult{matches, hasMore, nextCursor}
deactivate Storage
Handler-->>Client: SearchMatchesResponse{matches, hasMore, nextCursor}
deactivate Handler
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
yottahmd
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
🧹 Nitpick comments (2)
api/v1/api.yaml (1)
8457-8459: Reuse the canonical identifier schemas here.These values are what callers will round-trip into
/search/dags/{fileName}/matchesand/search/docs/matches?path=.... Keeping them as raw strings duplicates the constraints and descriptions that already live inDAGFileNameandDocPath.♻️ Suggested ref update
fileName: - type: string - description: "DAG file name without extension" + $ref: "#/components/schemas/DAGFileName" ... id: - type: string + $ref: "#/components/schemas/DocPath"Also applies to: 8500-8501
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/v1/api.yaml` around lines 8457 - 8459, The schema for the fileName and related string fields duplicates existing identifier schemas; replace the inline string schema/description for fileName with a $ref to the canonical DAGFileName schema (and similarly replace the other occurrences with a $ref to DocPath where appropriate) so callers use the same validation/description; update the schema node that currently defines fileName (and the corresponding field at the other occurrence) to reference DAGFileName or DocPath instead of defining type: string and duplicate description.ui/src/api/v1/schema.ts (1)
3446-3496: Align lazy-match pagination fields across both payload shapes.
DAGSearchPageItem/DocSearchPageItemusehasMoreMatchesandnextMatchesCursor, butSearchMatchesResponseswitches tohasMoreandnextCursor. That forces consumers to translate the load-more response before merging it back into item state. Since this file is generated, I'd normalize the names in the OpenAPI source and regenerate.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/src/api/v1/schema.ts` around lines 3446 - 3496, SearchMatchesResponse uses hasMore/nextCursor while DAGSearchPageItem and DocSearchPageItem use hasMoreMatches/nextMatchesCursor; update the OpenAPI source so SearchMatchesResponse (components.schemas.SearchMatchesResponse) uses hasMoreMatches and nextMatchesCursor to match DAGSearchPageItem and DocSearchPageItem, then regenerate the client to update ui/src/api/v1/schema.ts (look for SearchMatchesResponse, DAGSearchPageItem, DocSearchPageItem in the spec and ensure the pagination field names are identical).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@api/v1/api.yaml`:
- Around line 8510-8514: Rename the OpenAPI schema SearchDAGsMatchItem to a
neutral name SearchMatchItem and update all $ref usages that point to
"#/components/schemas/SearchDAGsMatchItem" (including the matches array items at
the occurrences you saw and the refs around lines 8559-8562) to
"#/components/schemas/SearchMatchItem"; ensure the schema block itself is
renamed to SearchMatchItem in the components/schemas section and run a quick
validation to confirm no remaining references to SearchDAGsMatchItem remain.
In `@internal/persis/filedoc/store.go`:
- Around line 618-622: The cursor pagination is unsafe because docSearchCursor
only stores the last emitted ID, but WalkDir traverses depth-first so IDs are
not monotonic; update the cursor and paging logic to record a stable traversal
key (e.g., the file relative path or a composite key like "path|id") instead of
only ID, and use that key to resume (or alternatively collect matching entries,
sort them by stable key such as filepath then ID, and paginate from that sorted
list). Specifically, extend docSearchCursor with a LastPath (or Key) field,
change the emit/resume checks in the search function that currently compare ID
to instead compare the stable key, and ensure any cursor versioning (Version) is
bumped/handled so old cursors fail gracefully.
- Around line 785-805: SearchMatches currently lets malformed IDs fall through
to docFilePath/os.ReadFile and can return filesystem errors instead of a proper
ErrInvalidDocID; add an upfront check for the doc id at the top of SearchMatches
(before calling decodeDocMatchCursor/docFilePath) and return
agent.ErrInvalidDocID for invalid IDs, using the existing doc-id validation
helper used by other store methods (e.g., validateDocID or agent.ValidateDocID)
so ID parsing/validation is centralized and consistent.
In `@internal/service/frontend/api/v1/docs_test.go`:
- Around line 220-277: The mock implementations of SearchCursor and
SearchMatches must mirror the real store: use
exec.EncodeSearchCursor/DecodeSearchCursor to decode incoming opts.Cursor and to
set NextCursor (not raw IDs or decimal offsets), and ensure SearchMatches first
finds the target document by id and returns an empty CursorResult (not
ErrDocNotFound) when the doc exists but has no matching hits; also guard against
negative offsets from strconv.Atoi by clamping to 0 and to len(matches) before
slicing. Update mockDocStore.SearchCursor and mockDocStore.SearchMatches to
decode/encode cursors with exec.DecodeSearchCursor/exec.EncodeSearchCursor,
lookup the document before paging, and clamp computed offsets to [0,len(results
or matches)].
In `@internal/service/frontend/api/v1/search.go`:
- Around line 83-91: The mapping sets both Name and FileName to item.FileName
because exec.SearchDAGResult currently lacks a separate display name; update
exec.SearchDAGResult to include a Name (or DisplayName) field and populate it
where SearchDAGResult values are constructed, then change toDAGSearchPageItem to
set Name: item.Name (or item.DisplayName) while keeping FileName: item.FileName
so api.DAGSearchPageItem reflects a proper display name distinct from the file
path; alternatively, if duplication was intentional, add a clarifying comment in
toDAGSearchPageItem explaining why Name mirrors FileName.
In `@ui/src/features/dags/components/common/DAGPagination.tsx`:
- Around line 23-25: DAGPagination currently renders the page-size selector even
when there is no onPageLimitChange handler, making the UI appear interactive
without effect; update the rendering conditions so the selector (and its
presets/custom entries) is only rendered/enabled when showPageLimitSelector is
true AND onPageLimitChange is provided—i.e., guard the selector render and any
interactive entries with a check like (showPageLimitSelector && typeof
onPageLimitChange === 'function') in the DAGPagination component so that all
places that render the page-size selector (including the blocks around the
presets/custom entries and any dropdown items) are hidden or disabled when the
handler is absent.
In `@ui/src/features/search/components/__tests__/SearchResult.test.tsx`:
- Around line 1-5: This test file (SearchResult.test.tsx) is missing the
required GPL v3 license header; fix it by adding the GPL v3 banner to the top of
the file (same style used across TS/TSX/JS sources) or simply run the repository
tooling: execute make addlicense in this directory to apply the correct header
automatically so the imports and symbols like SearchResult, render, screen,
userEvent remain unchanged.
In `@ui/src/features/search/components/SearchResult.tsx`:
- Around line 119-130: The header in the SearchResult component (the Link
wrapping the <h3> that renders title and kind) can overflow and push the matches
counter off-screen; update the Link container inside SearchResult to include a
min-w-0 utility and add whitespace-normal and break-words utilities to the
heading that renders title (the <h3> using title and kind) so long titles wrap
correctly and prevent layout overflow while preserving the existing
justify-between layout.
In `@ui/src/pages/search/index.tsx`:
- Around line 3-8: This TSX file is missing the required GPL v3 license header;
add the repository's standard GPL header at the very top of the file (before any
imports) by running the repo toolchain command `make addlicense` (preferred) or
by inserting the exact header used across the codebase, ensuring it appears
above the import block that contains symbols like SearchResult, useInfinite,
SearchIcon, useLocation/useSearchParams and ToggleButton/ToggleGroup so the file
complies with the project's licensing guidelines.
- Around line 426-477: The search controls currently use the last submitted
query (currentFilters.searchVal) and block empty submits; update the Input,
Button and ToggleButton handlers to use the live draft state (searchVal) and
allow empty submissions so users can clear searches: remove the trim-based guard
in the Input onKeyDown (call onSubmit(searchVal) on Enter unconditionally),
ensure the Button is not disabled based on searchVal.trim() (allow click to call
onSubmit(searchVal)), and change both ToggleButton onClick calls to pass
syncFilters({ searchVal: searchVal, scope: 'dags' }) / syncFilters({ searchVal:
searchVal, scope: 'docs' }) instead of currentFilters.searchVal; keep
buildSearchParams usage unchanged since it already supports empty queries.
---
Nitpick comments:
In `@api/v1/api.yaml`:
- Around line 8457-8459: The schema for the fileName and related string fields
duplicates existing identifier schemas; replace the inline string
schema/description for fileName with a $ref to the canonical DAGFileName schema
(and similarly replace the other occurrences with a $ref to DocPath where
appropriate) so callers use the same validation/description; update the schema
node that currently defines fileName (and the corresponding field at the other
occurrence) to reference DAGFileName or DocPath instead of defining type: string
and duplicate description.
In `@ui/src/api/v1/schema.ts`:
- Around line 3446-3496: SearchMatchesResponse uses hasMore/nextCursor while
DAGSearchPageItem and DocSearchPageItem use hasMoreMatches/nextMatchesCursor;
update the OpenAPI source so SearchMatchesResponse
(components.schemas.SearchMatchesResponse) uses hasMoreMatches and
nextMatchesCursor to match DAGSearchPageItem and DocSearchPageItem, then
regenerate the client to update ui/src/api/v1/schema.ts (look for
SearchMatchesResponse, DAGSearchPageItem, DocSearchPageItem in the spec and
ensure the pagination field names are identical).
🪄 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: CHILL
Plan: Pro
Run ID: ae710e19-1e0d-4436-a251-275e5ca4724f
📒 Files selected for processing (25)
api/v1/api.gen.goapi/v1/api.yamlinternal/agent/doc.gointernal/cmd/migrator_test.gointernal/cmn/telemetry/collector_test.gointernal/core/exec/dag.gointernal/core/exec/search_cursor.gointernal/core/exec/search_cursor_test.gointernal/persis/filedag/grep/grep.gointernal/persis/filedag/grep/grep_test.gointernal/persis/filedag/store.gointernal/persis/filedag/store_test.gointernal/persis/filedoc/store.gointernal/persis/filedoc/store_test.gointernal/runtime/agent/dbclient_test.gointernal/service/frontend/api/v1/docs_test.gointernal/service/frontend/api/v1/search.gointernal/service/frontend/api/v1/search_test.goui/src/api/v1/schema.tsui/src/features/dags/components/common/DAGPagination.tsxui/src/features/search/components/SearchResult.tsxui/src/features/search/components/__tests__/SearchResult.test.tsxui/src/pages/search/__tests__/index.test.tsxui/src/pages/search/index.tsxui/src/pages/setup.tsx
Comment on lines
+8510
to
+8514
| matches: | ||
| type: array | ||
| description: "Preview snippets for the result" | ||
| items: | ||
| $ref: "#/components/schemas/SearchDAGsMatchItem" |
There was a problem hiding this comment.
Use a neutral match-item schema for the shared doc/DAG responses.
SearchDAGsMatchItem is now referenced by document search too, so the generated docs and client types will still describe document hits as matches “within a DAG definition.” A generic SearchMatchItem would keep this shared contract accurate.
🧭 Suggested rename
matches:
type: array
description: "Preview snippets for the result"
items:
- $ref: "#/components/schemas/SearchDAGsMatchItem"
+ $ref: "#/components/schemas/SearchMatchItem"
...
matches:
type: array
items:
- $ref: "#/components/schemas/SearchDAGsMatchItem"
+ $ref: "#/components/schemas/SearchMatchItem"Rename the existing SearchDAGsMatchItem schema block to SearchMatchItem and update the other refs at the same time.
Also applies to: 8559-8562
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@api/v1/api.yaml` around lines 8510 - 8514, Rename the OpenAPI schema
SearchDAGsMatchItem to a neutral name SearchMatchItem and update all $ref usages
that point to "#/components/schemas/SearchDAGsMatchItem" (including the matches
array items at the occurrences you saw and the refs around lines 8559-8562) to
"#/components/schemas/SearchMatchItem"; ensure the schema block itself is
renamed to SearchMatchItem in the components/schemas section and run a quick
validation to confirm no remaining references to SearchDAGsMatchItem remain.
Comment on lines
+618
to
+622
| type docSearchCursor struct { | ||
| Version int `json:"v"` | ||
| Query string `json:"q"` | ||
| ID string `json:"id,omitempty"` | ||
| } |
There was a problem hiding this comment.
Cursor pagination can skip results for mixed file/directory prefixes.
This cursor only remembers the last emitted doc ID, but filepath.WalkDir resumes in depth-first directory order, not monotonically by doc ID. With both a.md and a/b.md, page 1 can end on a/b; page 2 then skips a because id <= cursor.ID, so that file is never returned. The cursor needs to track actual traversal position, or the search needs an explicit stable sort order before paging.
Also applies to: 713-748
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/persis/filedoc/store.go` around lines 618 - 622, The cursor
pagination is unsafe because docSearchCursor only stores the last emitted ID,
but WalkDir traverses depth-first so IDs are not monotonic; update the cursor
and paging logic to record a stable traversal key (e.g., the file relative path
or a composite key like "path|id") instead of only ID, and use that key to
resume (or alternatively collect matching entries, sort them by stable key such
as filepath then ID, and paginate from that sorted list). Specifically, extend
docSearchCursor with a LastPath (or Key) field, change the emit/resume checks in
the search function that currently compare ID to instead compare the stable key,
and ensure any cursor versioning (Version) is bumped/handled so old cursors fail
gracefully.
Comment on lines
+785
to
+805
| func (s *Store) SearchMatches(_ context.Context, id string, opts agent.SearchDocMatchesOptions) (*exec.CursorResult[*exec.Match], error) { | ||
| if opts.Query == "" { | ||
| return &exec.CursorResult[*exec.Match]{Items: []*exec.Match{}}, nil | ||
| } | ||
|
|
||
| cursor, err := decodeDocMatchCursor(opts.Cursor, opts.Query, id) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| path, err := s.docFilePath(id) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| data, err := os.ReadFile(path) //nolint:gosec // validated path within baseDir | ||
| if err != nil { | ||
| if os.IsNotExist(err) { | ||
| return nil, agent.ErrDocNotFound | ||
| } | ||
| return nil, err | ||
| } |
There was a problem hiding this comment.
Validate id before resolving the match path.
SearchMatches is the odd doc entry point out: the other store methods reject malformed IDs up front, but this one falls through to docFilePath/os.ReadFile and can return a path-traversal/internal filesystem error or ErrDocNotFound instead of ErrInvalidDocID.
Proposed fix
func (s *Store) SearchMatches(_ context.Context, id string, opts agent.SearchDocMatchesOptions) (*exec.CursorResult[*exec.Match], error) {
+ if err := agent.ValidateDocID(id); err != nil {
+ return nil, err
+ }
if opts.Query == "" {
return &exec.CursorResult[*exec.Match]{Items: []*exec.Match{}}, nil
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/persis/filedoc/store.go` around lines 785 - 805, SearchMatches
currently lets malformed IDs fall through to docFilePath/os.ReadFile and can
return filesystem errors instead of a proper ErrInvalidDocID; add an upfront
check for the doc id at the top of SearchMatches (before calling
decodeDocMatchCursor/docFilePath) and return agent.ErrInvalidDocID for invalid
IDs, using the existing doc-id validation helper used by other store methods
(e.g., validateDocID or agent.ValidateDocID) so ID parsing/validation is
centralized and consistent.
Comment on lines
+220
to
+277
| func (m *mockDocStore) SearchCursor(_ context.Context, opts agent.SearchDocsOptions) (*exec.CursorResult[agent.DocSearchResult], error) { | ||
| results, err := m.Search(context.Background(), opts.Query) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| limit := max(opts.Limit, 1) | ||
| offset := 0 | ||
| if opts.Cursor != "" { | ||
| for i, item := range results { | ||
| if item.ID == opts.Cursor { | ||
| offset = i + 1 | ||
| break | ||
| } | ||
| } | ||
| } | ||
| end := min(offset+limit, len(results)) | ||
| pageItems := make([]agent.DocSearchResult, 0, end-offset) | ||
| for _, item := range results[offset:end] { | ||
| pageItems = append(pageItems, *item) | ||
| } | ||
| result := &exec.CursorResult[agent.DocSearchResult]{ | ||
| Items: pageItems, | ||
| HasMore: end < len(results), | ||
| } | ||
| if result.HasMore && len(pageItems) > 0 { | ||
| result.NextCursor = pageItems[len(pageItems)-1].ID | ||
| } | ||
| return result, nil | ||
| } | ||
|
|
||
| func (m *mockDocStore) SearchMatches(_ context.Context, id string, opts agent.SearchDocMatchesOptions) (*exec.CursorResult[*exec.Match], error) { | ||
| results, err := m.Search(context.Background(), opts.Query) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| for _, result := range results { | ||
| if result.ID != id { | ||
| continue | ||
| } | ||
| limit := max(opts.Limit, 1) | ||
| offset := 0 | ||
| if opts.Cursor != "" { | ||
| if n, err := strconv.Atoi(opts.Cursor); err == nil { | ||
| offset = min(len(result.Matches), n) | ||
| } | ||
| } | ||
| end := min(offset+limit, len(result.Matches)) | ||
| cursorResult := &exec.CursorResult[*exec.Match]{ | ||
| Items: result.Matches[offset:end], | ||
| HasMore: end < len(result.Matches), | ||
| } | ||
| if cursorResult.HasMore { | ||
| cursorResult.NextCursor = strconv.Itoa(end) | ||
| } | ||
| return cursorResult, nil | ||
| } | ||
| return nil, agent.ErrDocNotFound | ||
| } |
There was a problem hiding this comment.
Make this mock mirror the real doc-search cursor contract.
SearchDocFeed/SearchDocMatches pass cursors through opaquely, but this mock uses raw IDs/decimal offsets instead of the encoded cursor format the real store returns. It also turns “document exists but query has no hits” into agent.ErrDocNotFound, and strconv.Atoi("-1") can drive offset negative and panic when slicing.
Please encode/decode with exec.{EncodeSearchCursor,DecodeSearchCursor} here as well, and look up the target doc first so a hitless doc returns an empty CursorResult.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/service/frontend/api/v1/docs_test.go` around lines 220 - 277, The
mock implementations of SearchCursor and SearchMatches must mirror the real
store: use exec.EncodeSearchCursor/DecodeSearchCursor to decode incoming
opts.Cursor and to set NextCursor (not raw IDs or decimal offsets), and ensure
SearchMatches first finds the target document by id and returns an empty
CursorResult (not ErrDocNotFound) when the doc exists but has no matching hits;
also guard against negative offsets from strconv.Atoi by clamping to 0 and to
len(matches) before slicing. Update mockDocStore.SearchCursor and
mockDocStore.SearchMatches to decode/encode cursors with
exec.DecodeSearchCursor/exec.EncodeSearchCursor, lookup the document before
paging, and clamp computed offsets to [0,len(results or matches)].
Comment on lines
+83
to
+91
| func toDAGSearchPageItem(item exec.SearchDAGResult) api.DAGSearchPageItem { | ||
| return api.DAGSearchPageItem{ | ||
| FileName: item.FileName, | ||
| Name: item.FileName, | ||
| HasMoreMatches: item.HasMoreMatches, | ||
| NextMatchesCursor: optionalString(item.NextMatchesCursor), | ||
| Matches: toSearchMatchItems(item.Matches), | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Check if exec.SearchDAGResult has a separate Name field that should be used.
ast-grep --pattern $'type SearchDAGResult struct {
$$$
}'Repository: dagu-org/dagu
Length of output: 369
🏁 Script executed:
ast-grep --pattern $'type DAGSearchPageItem struct {
$$$
}'Repository: dagu-org/dagu
Length of output: 973
Consider whether SearchDAGResult should have a separate Name field.
The Name field in api.DAGSearchPageItem is documented as "Display name for the DAG result," distinct from FileName. However, both are currently set to item.FileName because exec.SearchDAGResult lacks a separate Name field. Either add a Name field to SearchDAGResult to provide a proper display name, or clarify if this duplication is intentional.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/service/frontend/api/v1/search.go` around lines 83 - 91, The mapping
sets both Name and FileName to item.FileName because exec.SearchDAGResult
currently lacks a separate display name; update exec.SearchDAGResult to include
a Name (or DisplayName) field and populate it where SearchDAGResult values are
constructed, then change toDAGSearchPageItem to set Name: item.Name (or
item.DisplayName) while keeping FileName: item.FileName so api.DAGSearchPageItem
reflects a proper display name distinct from the file path; alternatively, if
duplication was intentional, add a clarifying comment in toDAGSearchPageItem
explaining why Name mirrors FileName.
Comment on lines
+23
to
+25
| onPageLimitChange?: (pageLimit: number) => void; | ||
| /** Whether to show the page-size selector */ | ||
| showPageLimitSelector?: boolean; |
There was a problem hiding this comment.
Hide the page-size selector when there is no change handler.
onPageLimitChange is optional now, but the selector still renders by default. In that state every preset/custom entry is a no-op, so the UI looks interactive while doing nothing.
♻️ Small fix
const DAGPagination = ({
totalPages,
page,
pageChange,
pageLimit,
onPageLimitChange,
- showPageLimitSelector = true,
+ showPageLimitSelector = !!onPageLimitChange,
}: DAGPaginationProps) => {
@@
- {showPageLimitSelector && (
+ {showPageLimitSelector && onPageLimitChange && (Also applies to: 217-218, 239-246, 270-321
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@ui/src/features/dags/components/common/DAGPagination.tsx` around lines 23 -
25, DAGPagination currently renders the page-size selector even when there is no
onPageLimitChange handler, making the UI appear interactive without effect;
update the rendering conditions so the selector (and its presets/custom entries)
is only rendered/enabled when showPageLimitSelector is true AND
onPageLimitChange is provided—i.e., guard the selector render and any
interactive entries with a check like (showPageLimitSelector && typeof
onPageLimitChange === 'function') in the DAGPagination component so that all
places that render the page-size selector (including the blocks around the
presets/custom entries and any dropdown items) are hidden or disabled when the
handler is absent.
Comment on lines
+1
to
+5
| import { render, screen, waitFor } from '@testing-library/react'; | ||
| import userEvent from '@testing-library/user-event'; | ||
| import { MemoryRouter } from 'react-router-dom'; | ||
| import { beforeEach, describe, expect, it, vi } from 'vitest'; | ||
| import SearchResult from '../SearchResult'; |
There was a problem hiding this comment.
Add the GPL header to this new test file.
This TSX file is missing the required license banner. Please run make addlicense here too.
As per coding guidelines, **/*.{go,ts,tsx,js}: Apply GPL v3 license headers on source files, managed via make addlicense.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@ui/src/features/search/components/__tests__/SearchResult.test.tsx` around
lines 1 - 5, This test file (SearchResult.test.tsx) is missing the required GPL
v3 license header; fix it by adding the GPL v3 banner to the top of the file
(same style used across TS/TSX/JS sources) or simply run the repository tooling:
execute make addlicense in this directory to apply the correct header
automatically so the imports and symbols like SearchResult, render, screen,
userEvent remain unchanged.
Comment on lines
+119
to
+130
| <div className="flex items-center justify-between gap-4"> | ||
| <Link to={link}> | ||
| <h3 className="text-lg font-semibold text-foreground"> | ||
| {title} | ||
| <span className="ml-2 rounded bg-muted px-1.5 py-0.5 text-xs font-normal text-muted-foreground"> | ||
| {kind} | ||
| </span> | ||
| </h3> | ||
| </Link> | ||
| <span className="text-xs text-muted-foreground"> | ||
| {matches.length} shown | ||
| </span> |
There was a problem hiding this comment.
Let long titles wrap instead of blowing out the result row.
The left side of this justify-between header can't shrink today, so long DAG names/doc titles can push the “shown” counter off-screen. Add min-w-0 on the link container and whitespace-normal break-words on the heading text.
🎨 Small layout fix
- <div className="flex items-center justify-between gap-4">
- <Link to={link}>
- <h3 className="text-lg font-semibold text-foreground">
+ <div className="flex items-start justify-between gap-4">
+ <Link to={link} className="block min-w-0">
+ <h3 className="text-lg font-semibold text-foreground whitespace-normal break-words">
{title}
<span className="ml-2 rounded bg-muted px-1.5 py-0.5 text-xs font-normal text-muted-foreground">
{kind}
</span>
</h3>
</Link>
- <span className="text-xs text-muted-foreground">
+ <span className="shrink-0 text-xs text-muted-foreground">
{matches.length} shown
</span>As per coding guidelines, ui/**/*.{ts,tsx}: Always handle long text in tables and lists with whitespace-normal break-words to prevent layout overflow.
📝 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.
Suggested change
| <div className="flex items-center justify-between gap-4"> | |
| <Link to={link}> | |
| <h3 className="text-lg font-semibold text-foreground"> | |
| {title} | |
| <span className="ml-2 rounded bg-muted px-1.5 py-0.5 text-xs font-normal text-muted-foreground"> | |
| {kind} | |
| </span> | |
| </h3> | |
| </Link> | |
| <span className="text-xs text-muted-foreground"> | |
| {matches.length} shown | |
| </span> | |
| <div className="flex items-start justify-between gap-4"> | |
| <Link to={link} className="block min-w-0"> | |
| <h3 className="text-lg font-semibold text-foreground whitespace-normal break-words"> | |
| {title} | |
| <span className="ml-2 rounded bg-muted px-1.5 py-0.5 text-xs font-normal text-muted-foreground"> | |
| {kind} | |
| </span> | |
| </h3> | |
| </Link> | |
| <span className="shrink-0 text-xs text-muted-foreground"> | |
| {matches.length} shown | |
| </span> |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@ui/src/features/search/components/SearchResult.tsx` around lines 119 - 130,
The header in the SearchResult component (the Link wrapping the <h3> that
renders title and kind) can overflow and push the matches counter off-screen;
update the Link container inside SearchResult to include a min-w-0 utility and
add whitespace-normal and break-words utilities to the heading that renders
title (the <h3> using title and kind) so long titles wrap correctly and prevent
layout overflow while preserving the existing justify-between layout.
Comment on lines
+3
to
+8
| import SearchResult from '@/features/search/components/SearchResult'; | ||
| import { useInfinite } from '@/hooks/api'; | ||
| import { Search as SearchIcon } from 'lucide-react'; | ||
| import React, { useEffect, useRef } from 'react'; | ||
| import { useSearchParams } from 'react-router-dom'; | ||
| import React, { useEffect, useMemo, useRef } from 'react'; | ||
| import { useLocation, useSearchParams } from 'react-router-dom'; | ||
| import { ToggleButton, ToggleGroup } from '../../components/ui/toggle-group'; |
There was a problem hiding this comment.
Add the required GPL header to this TSX source file.
This file is being modified, but it still lacks the repository's license header. Please run make addlicense as part of this change.
As per coding guidelines, "Apply GPL v3 license headers on source files, managed via make addlicense".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@ui/src/pages/search/index.tsx` around lines 3 - 8, This TSX file is missing
the required GPL v3 license header; add the repository's standard GPL header at
the very top of the file (before any imports) by running the repo toolchain
command `make addlicense` (preferred) or by inserting the exact header used
across the codebase, ensuring it appears above the import block that contains
symbols like SearchResult, useInfinite, SearchIcon, useLocation/useSearchParams
and ToggleButton/ToggleGroup so the file complies with the project's licensing
guidelines.
Comment on lines
+426
to
+477
| <Input | ||
| placeholder="Search text..." | ||
| className="max-w-md" | ||
| ref={inputRef} | ||
| value={searchVal} | ||
| onChange={(e) => { | ||
| setSearchVal(e.target.value); | ||
| }} | ||
| type="search" | ||
| onKeyDown={(e) => { | ||
| if (e.key === 'Enter' && searchVal.trim()) { | ||
| onSubmit(searchVal); | ||
| } | ||
| } | ||
| }} | ||
| /> | ||
| <Button | ||
| disabled={!searchVal} | ||
| onClick={async () => { | ||
| onSubmit(searchVal); | ||
| }} | ||
| > | ||
| <SearchIcon className="h-4 w-4" /> | ||
| Search | ||
| </Button> | ||
| }} | ||
| /> | ||
| <Button | ||
| disabled={!searchVal.trim()} | ||
| onClick={() => { | ||
| onSubmit(searchVal); | ||
| }} | ||
| > | ||
| <SearchIcon className="h-4 w-4" /> | ||
| Search | ||
| </Button> | ||
| </div> | ||
|
|
||
| <ToggleGroup aria-label="Search scope"> | ||
| <ToggleButton | ||
| value="dags" | ||
| groupValue={currentFilters.scope} | ||
| onClick={() => { | ||
| syncFilters({ | ||
| searchVal: currentFilters.searchVal, | ||
| scope: 'dags', | ||
| }); | ||
| }} | ||
| > | ||
| DAGs | ||
| </ToggleButton> | ||
| <ToggleButton | ||
| value="docs" | ||
| groupValue={currentFilters.scope} | ||
| onClick={() => { | ||
| syncFilters({ | ||
| searchVal: currentFilters.searchVal, | ||
| scope: 'docs', | ||
| }); | ||
| }} | ||
| > | ||
| Docs | ||
| </ToggleButton> | ||
| </ToggleGroup> |
There was a problem hiding this comment.
The search controls are wired to the last submitted query, not the draft input.
Blocking empty submit means users can't clear an existing search, and the scope toggles re-submit currentFilters.searchVal, which overwrites any text typed after the last submit. buildSearchParams() already supports the empty-query state, so these controls should submit the current searchVal consistently.
Proposed fix
<Input
placeholder="Search text..."
className="max-w-md"
ref={inputRef}
value={searchVal}
onChange={(e) => {
setSearchVal(e.target.value);
}}
type="search"
onKeyDown={(e) => {
- if (e.key === 'Enter' && searchVal.trim()) {
+ if (e.key === 'Enter') {
onSubmit(searchVal);
}
}}
/>
<Button
- disabled={!searchVal.trim()}
+ disabled={!searchVal.trim() && !submittedQuery}
onClick={() => {
onSubmit(searchVal);
}}
>
<SearchIcon className="h-4 w-4" />
Search
@@
<ToggleButton
value="dags"
groupValue={currentFilters.scope}
onClick={() => {
syncFilters({
- searchVal: currentFilters.searchVal,
+ searchVal,
scope: 'dags',
});
}}
>
@@
<ToggleButton
value="docs"
groupValue={currentFilters.scope}
onClick={() => {
syncFilters({
- searchVal: currentFilters.searchVal,
+ searchVal,
scope: 'docs',
});
}}
>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@ui/src/pages/search/index.tsx` around lines 426 - 477, The search controls
currently use the last submitted query (currentFilters.searchVal) and block
empty submits; update the Input, Button and ToggleButton handlers to use the
live draft state (searchVal) and allow empty submissions so users can clear
searches: remove the trim-based guard in the Input onKeyDown (call
onSubmit(searchVal) on Enter unconditionally), ensure the Button is not disabled
based on searchVal.trim() (allow click to call onSubmit(searchVal)), and change
both ToggleButton onClick calls to pass syncFilters({ searchVal: searchVal,
scope: 'dags' }) / syncFilters({ searchVal: searchVal, scope: 'docs' }) instead
of currentFilters.searchVal; keep buildSearchParams usage unchanged since it
already supports empty queries.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #1945 +/- ##
==========================================
+ Coverage 68.60% 68.61% +0.01%
==========================================
Files 462 463 +1
Lines 58359 58751 +392
==========================================
+ Hits 40035 40313 +278
- Misses 14636 14706 +70
- Partials 3688 3732 +44
... and 13 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Testing
make apipnpm gen:apimake fmtgo test ./internal/core/exec ./internal/persis/filedag ./internal/persis/filedoc ./internal/service/frontend/api/v1 -count=1pnpm typecheckpnpm test -- --run ui/src/pages/search/__tests__/index.test.tsx ui/src/features/search/components/__tests__/SearchResult.test.tsxpnpm build