feat: [ENG-1896] Implement QueryLogUseCase with list and detail views#349
feat: [ENG-1896] Implement QueryLogUseCase with list and detail views#349bao-byterover merged 2 commits intoproj/query-logfrom
Conversation
|
Claude finished @bao-byterover's task in 2m 31s —— View job PR Review: feat: [ENG-1896] Implement QueryLogUseCase
Overall Assessment: 4 / 5Well-executed presentation layer. TDD is clearly followed (16 tests, all pass), Outside-In approach is correctly applied (store interface derived from use case needs, not the other way around), and the scope boundary is properly enforced — no store implementation, no oclif changes. No security issues. The code is output-only, all data comes from a trusted local store, and Issues Found
Highlights
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| } | ||
|
|
||
| export function formatEntryDuration(entry: QueryLogEntry): string { | ||
| if (entry.status !== 'completed') return '—' |
There was a problem hiding this comment.
issue (design inconsistency): formatEntryDuration returns '—' for error entries in the list view, but showDetail calls formatDuration(entry.startedAt, entry.completedAt) directly (line 88 of the use case), which will show the real duration for cancelled and error entries in detail view.
An error entry that took 2s shows — in the list but 2.0s in the detail. This asymmetry is tested/documented ("error entry shows '—' for Time"), but it's a design trap — if a future maintainer adds 'cancelled' or 'error' to the condition, the list and detail views will still diverge because they use different code paths.
Consider either:
- Keeping the intentional
—for non-completed entries in both views (updateshowDetailto useformatEntryDurationtoo), or - Accepting the asymmetry and documenting it explicitly here.
| if (entry.status !== 'completed') return '—' | |
| export function formatEntryDuration(entry: QueryLogEntry): string { | |
| if (entry.status !== 'completed') return '—' | |
| return formatDuration(entry.startedAt, entry.completedAt) | |
| } |
| async run({ | ||
| after, | ||
| before, | ||
| detail = false, | ||
| format = 'text', | ||
| id, | ||
| limit = 10, | ||
| status, | ||
| tier, | ||
| }: { | ||
| after?: number | ||
| before?: number | ||
| detail?: boolean | ||
| format?: 'json' | 'text' | ||
| id?: string | ||
| limit?: number | ||
| status?: QueryLogStatus[] | ||
| tier?: QueryLogTier[] | ||
| }): Promise<void> { |
There was a problem hiding this comment.
suggestion (DRY): The run method redeclares the full options type inline, duplicating the IQueryLogUseCase interface. If the interface gains or drops a parameter, this must be updated separately — and since TypeScript structural typing won't catch a missing optional param, drift could go unnoticed.
| async run({ | |
| after, | |
| before, | |
| detail = false, | |
| format = 'text', | |
| id, | |
| limit = 10, | |
| status, | |
| tier, | |
| }: { | |
| after?: number | |
| before?: number | |
| detail?: boolean | |
| format?: 'json' | 'text' | |
| id?: string | |
| limit?: number | |
| status?: QueryLogStatus[] | |
| tier?: QueryLogTier[] | |
| }): Promise<void> { | |
| async run(options: Parameters<IQueryLogUseCase['run']>[0]): Promise<void> { | |
| const { | |
| after, | |
| before, | |
| detail = false, | |
| format = 'text', | |
| id, | |
| limit = 10, | |
| status, | |
| tier, | |
| } = options | |
| await (id | |
| ? this.showDetail(id, format) | |
| : this.showList({after, before, detail, format, limit, status, tier})) | |
| } |
| ].join(' ') | ||
| this.log(row) | ||
|
|
||
| if (detail && entry.status === 'completed' && entry.matchedDocs.length > 0) { |
There was a problem hiding this comment.
nitpick: detail mode in list view only inlines matched docs — the parameter name implies general detail expansion (timestamps, error messages, etc.) but the implementation is narrower. This could confuse callers who pass detail: true expecting richer output for error or cancelled rows.
If the scope is intentionally "matched docs only for now", consider either:
- Renaming to
showMatchedDocsfor clarity, or - Adding a
// TODO(ENG-XXXX): expand detail to include error messages and timestampscomment documenting the intentional limitation.
| ].join(' ') | ||
|
|
||
| this.log(header) | ||
| this.log('─'.repeat(idWidth + tierWidth + statusWidth + timeWidth + queryWidth + 8)) |
There was a problem hiding this comment.
nitpick: The separator width calculation (idWidth + tierWidth + statusWidth + timeWidth + queryWidth + 8) is correct (4 separators × 2 spaces = 8), but queryWidth isn't actually used in the separator — the Query column can overflow queryWidth if a padded entry is longer. This works fine today since the last column isn't padded, but a comment explaining the +8 would help future column additions.
| this.log('─'.repeat(idWidth + tierWidth + statusWidth + timeWidth + queryWidth + 8)) | |
| this.log('─'.repeat(idWidth + tierWidth + statusWidth + timeWidth + queryWidth + 8)) // +8 = 4 columns × 2-space separator |
| expect(output).to.include('T2') | ||
| expect(output).to.include('error') | ||
| expect(output).to.include('—') | ||
| }) |
There was a problem hiding this comment.
issue (coverage gap): The cancelled status variant is part of the discriminated union and has its own unique shape (completedAt present, no error, no matchedDocs), but there's no test for how a cancelled entry renders in either list or detail view.
In the list view, formatEntryDuration returns '—' for cancelled (same as error). In detail view, the entry.status !== 'processing' guard will show Finished and Duration for a cancelled entry — but no test verifies this. Adding a makeCancelledEntry helper and at least one list + one detail test would complete the coverage for the discriminated union.
|
Claude finished @bao-byterover's task in 2m 20s —— View job PR Review: feat: [ENG-1896] Implement QueryLogUseCase
Overall Assessment: 4.5 / 5The "Fix review" commit (0315068) properly addresses both critical issues from the previous round. The remaining items are a suggestion and two nitpicks — all carried over and still open. No security issues. Output-only code with no user-controlled format strings or injection vectors. What Was Fixed in the Latest Commit
Duration fix ( Cancelled coverage:
Total test count is now 19 (up from 16). Still Open
Highlights
|
| limit?: number | ||
| status?: QueryLogStatus[] | ||
| tier?: QueryLogTier[] | ||
| }): Promise<void> { |
There was a problem hiding this comment.
suggestion (DRY — still open from previous review): The run method's parameter type is still declared inline, duplicating IQueryLogUseCase['run']'s signature. If the interface gains a new option, this must be updated separately and TypeScript won't flag a missing optional param.
| }): Promise<void> { | |
| async run(options: Parameters<IQueryLogUseCase['run']>[0]): Promise<void> { | |
| const { | |
| after, | |
| before, | |
| detail = false, | |
| format = 'text', | |
| id, | |
| limit = 10, | |
| status, | |
| tier, | |
| } = options |
| ].join(' ') | ||
|
|
||
| this.log(header) | ||
| this.log('─'.repeat(idWidth + tierWidth + statusWidth + timeWidth + queryWidth + 8)) |
There was a problem hiding this comment.
nitpick (still open from previous review): The +8 magic number is correct (4 column separators × 2 spaces each), but unexplained. A future maintainer adding a column will likely copy the pattern without understanding why.
| this.log('─'.repeat(idWidth + tierWidth + statusWidth + timeWidth + queryWidth + 8)) | |
| this.log('─'.repeat(idWidth + tierWidth + statusWidth + timeWidth + queryWidth + 8)) // +8 = 4 column separators × 2 spaces |
| ].join(' ') | ||
| this.log(row) | ||
|
|
||
| if (detail && entry.status === 'completed' && entry.matchedDocs.length > 0) { |
There was a problem hiding this comment.
nitpick (still open from previous review): detail: true in list mode only expands matched docs. The parameter name implies general detail expansion (timestamps, error messages, etc.) but the implementation is narrower. Consider a comment to document the intentional scope limit:
| if (detail && entry.status === 'completed' && entry.matchedDocs.length > 0) { | |
| if (detail && entry.status === 'completed' && entry.matchedDocs.length > 0) { // detail: inline matched docs only |
* feat: [ENG-1897] Create brv query-log view oclif command (#347) * feat: [ENG-1897] Create brv query-log view oclif command * feat: [ENG-1897] fix review * feat: [ENG-1896] Implement QueryLogUseCase with list and detail views (#349) * feat: [ENG-1896] Implement QueryLogUseCase with list and detail views * feat: [ENG-1896] Fix review * feat: [ENG-1888] Define IQueryLogStore interface (#351) * feat: [ENG-1887] Define QueryLogEntry entity with discriminated union (#353) * feat: [ENG-1889] Implement FileQueryLogStore with Zod validation (#354) * feat: [ENG-1889] Implement FileQueryLogStore with Zod validation * feat: [ENG-1889] fix review * feat: [ENG-1899] Create brv query-log summary oclif command (#357) * feat: [ENG-1899] Create brv query-log summary oclif command * feat: [ENG-1899] fix review * feat: [ENG-1898] Implement QueryLogSummaryUseCase (#368) * feat: [ENG-1892] Add QueryExecutorResult type with tier and timing metadata (#369) * feat: [ENG-1893] Wire QueryLogHandler into daemon lifecycle (#370) * feat: [ENG-1894] Wire QueryLogHandler into daemon lifecycle (#371) * feat: [ENG-1894] enhance message output * feat: [proj/query-log] fix review * feat: [ENG-2123] brv curate view / brv query-log view truncate long c… (#424) * feat: [ENG-2123] brv curate view / brv query-log view truncate long content * feat: [ENG-2123] fix review * feat: [ENG-2177] Increase Storage Limit for Curate Log and Query Log (#430) * feat: [ENG-2177] Increase Storage Limit for Curate Log and Query Log * feat: [ENG-2177] fix review * feat: [ENG-2177] fix slow test --------- Co-authored-by: Hoang Pham <lehoangpham1092@gmail.com>
1 participant
Summary
brv query-log viewcommand (ENG-1897) has a stubQueryLogUseCasethat produces no output — the use case, entity, and store interface are empty shells.QueryLogUseCasewith list view (aligned table) and detail view (full entry display). DefinedQueryLogEntrydiscriminated union,IQueryLogStoreinterface, andITerminaloutput port. Extracted shared formatting utilities tolog-format-utils.ts. UpdatedFileQueryLogStorestub to satisfy the interface.Type of change
Scope (select all touched areas)
Linked issues
Root cause (bug fixes only, otherwise write
N/A)N/A
Test plan
test/unit/infra/usecase/query-log-use-case.test.tsUser-visible changes
None yet —
FileQueryLogStoreis still a stub returning empty data. Once the store is implemented (ENG-1889),brv query-log viewwill display:command,success,data,retrievedAtfieldsEvidence
Before (stub): 16 failing, 0 passing
After (implementation): 0 failing, 16 passing
Checklist
npm test)npm run lint)npm run typecheck)npm run build)mainRisks and mitigations
QueryLogEntrydiscriminated union fields may not match whatFileQueryLogStore(ENG-1889) actually persists.