Skip to content

fix worker buffer size#408

Merged
SuperCoolPencil merged 2 commits intomainfrom
fix-worker-buffer-size
Apr 23, 2026
Merged

fix worker buffer size#408
SuperCoolPencil merged 2 commits intomainfrom
fix-worker-buffer-size

Conversation

@SuperCoolPencil
Copy link
Copy Markdown
Member

@SuperCoolPencil SuperCoolPencil commented Apr 23, 2026

  • feat: add support for KB-scaled float values to worker_buffer_size setting
  • test: add unit tests for TUI settings value conversion and validation

Greptile Summary

This PR adds KB-scaled input support for the worker_buffer_size TUI setting, converting the user-entered KB value to bytes via ParseFloat before storing it — mirroring the existing MB conversion for min_chunk_size. A new test file covers the round-trip conversion for several settings, including worker_buffer_size.

Confidence Score: 5/5

Safe to merge; the conversion logic is correct and consistent with the existing pattern for min_chunk_size.

The only finding is a P2 suggestion to extend the new test with edge-case inputs (zero, invalid string, fractional KB). No logic errors or breaking changes were identified.

internal/tui/settings_unit_test.go — consider adding edge-case tests for the worker_buffer_size conversion.

Important Files Changed

Filename Overview
internal/tui/view_settings.go Adds KB-to-bytes conversion for worker_buffer_size in setSettingValue; logic is correct but parse errors are silently swallowed (consistent with surrounding code).
internal/tui/settings_unit_test.go New test file covering several settings conversions; worker_buffer_size test only covers the happy path, missing edge cases (zero, invalid string, fractional input).
Prompt To Fix All With AI
This is a comment left during a code review.
Path: internal/tui/settings_unit_test.go
Line: 33-47

Comment:
**Missing edge-case tests for the new KB conversion path**

The `WorkerBufferSize` test case only exercises the happy path (`1024 KB` → valid positive integer). The custom rule for edge-case coverage requires testing boundary inputs as well. Notably:

- `"0"` → sets the field to `0` bytes, silently relying on `GetWorkerBufferSize()` to fall back to the default; there is no user-visible error.
- `"abc"` (non-numeric) → `ParseFloat` fails, the field is silently left unchanged, and `setSettingValue` still returns `nil`.
- Fractional input like `"1.5"` → accepted and silently truncated to `1536` bytes without feedback.

Adding tests for these cases would verify both the conversion logic and the error-handling (or lack thereof) behaviour.

**Rule Used:** What: All code changes must include tests for edge... ([source](https://app.greptile.com/review/custom-context?memory=2b22782d-3452-4d55-b059-e631b2540ce8))

How can I resolve this? If you propose a fix, please make it concise.

Reviews (1): Last reviewed commit: "test: add unit tests for TUI settings va..." | Re-trigger Greptile

Greptile also left 1 inline comment on this PR.

Context used:

  • Rule used - What: All code changes must include tests for edge... (source)

Comment on lines +33 to +47
},
{
name: "WorkerBufferSize KB Conversion",
category: "Network",
key: "worker_buffer_size",
typ: "int",
internalValue: 1024 * config.KB,
uiInput: "1024",
expectedValue: 1024 * config.KB,
},
{
name: "SlowWorkerGracePeriod seconds Conversion",
category: "Performance",
key: "slow_worker_grace_period",
typ: "duration",
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.

P2 Missing edge-case tests for the new KB conversion path

The WorkerBufferSize test case only exercises the happy path (1024 KB → valid positive integer). The custom rule for edge-case coverage requires testing boundary inputs as well. Notably:

  • "0" → sets the field to 0 bytes, silently relying on GetWorkerBufferSize() to fall back to the default; there is no user-visible error.
  • "abc" (non-numeric) → ParseFloat fails, the field is silently left unchanged, and setSettingValue still returns nil.
  • Fractional input like "1.5" → accepted and silently truncated to 1536 bytes without feedback.

Adding tests for these cases would verify both the conversion logic and the error-handling (or lack thereof) behaviour.

Rule Used: What: All code changes must include tests for edge... (source)

Prompt To Fix With AI
This is a comment left during a code review.
Path: internal/tui/settings_unit_test.go
Line: 33-47

Comment:
**Missing edge-case tests for the new KB conversion path**

The `WorkerBufferSize` test case only exercises the happy path (`1024 KB` → valid positive integer). The custom rule for edge-case coverage requires testing boundary inputs as well. Notably:

- `"0"` → sets the field to `0` bytes, silently relying on `GetWorkerBufferSize()` to fall back to the default; there is no user-visible error.
- `"abc"` (non-numeric) → `ParseFloat` fails, the field is silently left unchanged, and `setSettingValue` still returns `nil`.
- Fractional input like `"1.5"` → accepted and silently truncated to `1536` bytes without feedback.

Adding tests for these cases would verify both the conversion logic and the error-handling (or lack thereof) behaviour.

**Rule Used:** What: All code changes must include tests for edge... ([source](https://app.greptile.com/review/custom-context?memory=2b22782d-3452-4d55-b059-e631b2540ce8))

How can I resolve this? If you propose a fix, please make it concise.

@SuperCoolPencil SuperCoolPencil merged commit 8a4249b into main Apr 23, 2026
16 checks passed
@SuperCoolPencil SuperCoolPencil deleted the fix-worker-buffer-size branch April 23, 2026 04:36
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