Skip to content

feat: [ENG-2123] brv curate view / brv query-log view truncate long c…#424

Merged
bao-byterover merged 2 commits intoproj/query-logfrom
feat/ENG-2123
Apr 15, 2026
Merged

feat: [ENG-2123] brv curate view / brv query-log view truncate long c…#424
bao-byterover merged 2 commits intoproj/query-logfrom
feat/ENG-2123

Conversation

@bao-byterover
Copy link
Copy Markdown
Collaborator

No description provided.

@bao-byterover bao-byterover requested a review from leehpham as a code owner April 15, 2026 15:53
@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Apr 15, 2026

Claude finished @bao-byterover's task in 2m 0s —— View job


PR Review: feat: [ENG-2123] brv curate view / brv query-log view — remove truncation in detail view

  • Gather context and read changed files
  • Analyze code changes for correctness, security, and quality
  • Post inline comments for specific issues
  • Post general summary with overall score

Summary

This PR removes truncation from the detail view (brv curate view <id> and brv query-log view <id>) for both context (was capped at 200 chars) and response (was capped at 500 chars). The list/table view still correctly truncates the query column to fit column width — that truncate() usage is intentional and untouched.

The changes are small, correct, and properly tested using TDD. Tests were updated before or alongside the implementation changes, following the project's strict TDD requirement.


Findings

Correctness — No bugs introduced. The logic simplification is straightforward: remove inline .slice()/truncate() calls and let the string pass through directly.

Security — No concerns. This is terminal output formatting with no user input reaching execution paths.

Dead importtruncate in query-log-use-case.ts is not dead; it's still used in showList() for the query column. Import is fine.

Inline comments posted:

  • query-log-use-case.ts:118 — multiline response indentation: only the first line gets the two-space indent when entry.response contains \n (suggestion, non-blocking)
  • curate-log-use-case.ts:113 — same multiline indentation concern for context (suggestion, non-blocking)
  • curate-log-use-case.test.ts:247 — semantic mismatch using makeProcessingEntry to test context display (nitpick, non-blocking)

Minor observation — PR title: The title reads "truncate long content" but the actual change removes truncation. The intent is clear from the diff, but the wording is inverted. Not a blocker.


Overall Score: 4 / 5

Clean, focused, well-tested change. The only non-trivial concern is the multiline indentation gap now exposed by removing truncation — worth a follow-up if responses or context values are ever multi-line in practice.

this.log('Response (truncated):')
this.log(` ${truncate(entry.response, 500)}`)
this.log('Response:')
this.log(` ${entry.response}`)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

suggestion (non-blocking): If entry.response contains newlines, only the first line will receive the two-space indent. This was less visible before since truncation would clip multi-paragraph responses. You may want to indent every line:

Suggested change
this.log(` ${entry.response}`)
this.log(entry.response.split('\n').map(line => ` ${line}`).join('\n'))

Low priority — depends on whether responses are ever multi-line in practice.

Comment thread src/server/infra/usecase/curate-log-use-case.ts Outdated
Comment thread test/unit/infra/usecase/curate-log-use-case.test.ts Outdated
@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Apr 15, 2026

Claude finished @bao-byterover's task in 2m 6s —— View job


PR Review: feat: [ENG-2123] brv curate view / brv query-log view — multi-line indentation fix

  • Gather context and read changed files
  • Analyze code changes for correctness, security, and quality
  • Post inline comments for specific issues
  • Post general summary with overall score

Summary

This revision directly addresses all three suggestions raised in the previous review cycle:

Previous concern Resolved?
query-log-use-case.ts — only first line of response was indented ✅ Fixed with split/map/join
curate-log-use-case.ts — only first line of context was indented ✅ Fixed with split + multi-call loop
curate-log-use-case.test.tsmakeProcessingEntry semantic mismatch ✅ Replaced with makeCompletedEntry

New tests (4 added across both test files) cover the corrected behaviour: full content with no truncation, correct header label (Response: not Response (truncated):), and proper two-space indentation of every continuation line.


Findings

Correctness — All changes are correct. Both the split-and-loop pattern (context) and the split/map/join pattern (response) produce identical terminal output. No regressions.

Security — No concerns. Terminal formatting, no execution paths.

Dead importtruncate in query-log-use-case.ts is still used in showList() at line 167 (truncate(entry.query, queryWidth)). Import is fine.

Inline comment posted:

  • curate-log-use-case.ts:141 — minor style inconsistency: context uses multi-call approach, response uses single-call with embedded \n. Both produce the same output; just a consistency note, non-blocking.

No other issues found.


Overall Score: 5 / 5

All previous feedback addressed. The implementation is clean, correct, and well-tested. The only note is a cosmetic style inconsistency between the two multi-line formatting approaches within curate-log-use-case.ts, which has no behavioural impact.

Comment thread src/server/infra/usecase/curate-log-use-case.ts
@bao-byterover bao-byterover merged commit 40d5527 into proj/query-log Apr 15, 2026
6 checks passed
bao-byterover added a commit that referenced this pull request Apr 16, 2026
* 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>
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.

1 participant