fix(format): fix WidthStrategy edge cases and reduce allocations

[apstndb]

Summary

Fix unresolved review issues from #569 and reduce allocations in GreedyFrequencyStrategy. Addresses two bugs (panic risk, unused screen width) and one performance improvement.

Key Changes

• width_strategy_greedy.go: Add empty headers guard to prevent panic. Replace lo.Must(lo.Nth(in, columnIdx)) with direct index access + bounds check to prevent panic on short rows. Replace hiter.Map/hiter.Concat/slices.Collect chain in transposedRows with direct slice construction. Replace lo.Sum with inline sumWidths to avoid evaluation on every slog.Debug call. Always distribute remainder (fall back to column 0 instead of skipping when idx == -1).
• width_strategy_proportional.go: Restructure distribution block so remaining width is assigned even when totalDeficit == 0 (all columns already fit).
• width.go: Optimize adjustToSum to compute sorted unique thresholds once and inline clipping + sum. Remove unused clipToMax function.
• width_strategy_test.go: Add TestGreedyFrequencyStrategy_ShortRows, TestProportionalStrategy_ZeroDeficit, TestGreedyFrequencyStrategy_FullWidth edge case tests.

Benchmark

Large (10 cols x 200 rows) Greedy allocations: 8865 → 8492 allocs/op.

Test Plan

• make check passes (test + lint + fmt-check)
• All existing width strategy tests pass unchanged
• New edge case tests pass
• Benchmarks show allocation reduction

Fixes #570, fixes #571

[gemini-code-assist]

Summary of Changes

Hello, I'm Gemini Code Assist¹! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!

This pull request enhances the table formatting logic by addressing several edge cases and improving performance. It resolves potential panics when handling short rows, ensures optimal use of available screen width across different strategies, and reduces memory allocations in the greedy width calculation, leading to a more stable and efficient display of tabular data.

Highlights

• GreedyFrequencyStrategy: Replaced lo.Must(lo.Nth(in, columnIdx)) with direct index access and bounds check to prevent panics on short rows. Replaced hiter.Map/hiter.Concat/slices.Collect chain with direct slice construction for transposedRows. Replaced lo.Sum with inline sumWidths to avoid closure evaluation in slog.Debug. Ensured remainder is always distributed, falling back to column 0 instead of skipping when idx == -1.
• ProportionalStrategy: Restructured the distribution block to assign remaining width even when totalDeficit is 0 (all columns already fit).
• Width Calculation: Optimized adjustToSum to compute sorted unique thresholds once and inline clipping and sum. Removed the unused clipToMax function.
• Tests: Added new edge case tests: TestGreedyFrequencyStrategy_ShortRows, TestProportionalStrategy_ZeroDeficit, and TestGreedyFrequencyStrategy_FullWidth.

Changelog

• internal/mycli/format/width.go
  • Removed unused clipToMax function.
  • Optimized adjustToSum to pre-calculate sorted unique thresholds and use direct clipping and summing.
• internal/mycli/format/width_strategy_greedy.go
  • Removed math import.
  • Introduced an inline sumWidths function to replace lo.Sum for performance and to avoid closure evaluation in slog.Debug.
  • Refactored transposedRows creation to use direct slice construction, preventing panics with short rows and reducing allocations.
  • Modified the logic for distributing remaining width to always fall back to column 0 if no specific shortage is found, ensuring full width utilization.
• internal/mycli/format/width_strategy_proportional.go
  • Restructured the proportional distribution logic to ensure remaining width is assigned even when totalDeficit is zero, guaranteeing full available width usage.
• internal/mycli/format/width_strategy_test.go
  • Added TestGreedyFrequencyStrategy_ShortRows to verify handling of rows with fewer columns than headers.
  • Added TestProportionalStrategy_ZeroDeficit to ensure proportional strategy uses full width when columns already fit.
  • Added TestGreedyFrequencyStrategy_FullWidth to ensure greedy strategy uses full width when columns already fit.

Activity

• Addressed unresolved review issues from PR feat(format): add WidthStrategy interface for pluggable column width algorithms #569.
• Resolved issues Fix unresolved review issues in WidthStrategy implementations #570 and Reduce allocations in GreedyFrequencyStrategy #571.
• Confirmed make check passes, including tests, lint, and format checks.
• Verified all existing width strategy tests pass.
• Ensured new edge case tests pass.
• Confirmed benchmarks show a reduction in memory allocations.

Using Gemini Code Assist

The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.

Invoking Gemini

You can request assistance from Gemini at any point by creating a comment using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.

Feature	Command	Description
Code Review	/gemini review	Performs a code review for the current pull request in its current state.
Pull Request Summary	/gemini summary	Provides a summary of the current pull request in its current state.
Comment	@gemini-code-assist	Responds in comments when explicitly tagged, both in pull request comments and review comments.
Help	/gemini help	Displays a list of available commands.

Customization

To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.

Limitations & Feedback

Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here.

You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.

Footnotes

1. Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩

[github-actions]

Code Metrics Report

📊 View detailed coverage report (available for 7 days)

	main (41f4265)	#572 (67beccb)	+/-
Coverage	70.4%	70.6%	+0.1%
Code to Test Ratio	1:1.2	1:1.2	+0.0
Test Execution Time	2m31s	1m10s	-1m21s

Details

  |                     | main (41f4265) | #572 (67beccb) |  +/-   |
  |---------------------|----------------|----------------|--------|
+ | Coverage            |          70.4% |          70.6% |  +0.1% |
  |   Files             |             74 |             74 |      0 |
  |   Lines             |           6921 |           6944 |    +23 |
+ |   Covered           |           4879 |           4906 |    +27 |
+ | Code to Test Ratio  |          1:1.2 |          1:1.2 |   +0.0 |
  |   Code              |          16137 |          16154 |    +17 |
+ |   Test              |          20769 |          20820 |    +51 |
+ | Test Execution Time |          2m31s |          1m10s | -1m21s |

Code coverage of files in pull request scope (89.1% → 90.9%)

Files	Coverage	+/-	Status
internal/mycli/execute_sql.go	82.6%	+1.3%	affected
internal/mycli/format/width.go	97.4%	+1.4%	modified
internal/mycli/format/width_strategy_greedy.go	97.9%	-2.1%	modified
internal/mycli/format/width_strategy_proportional.go	100.0%	0.0%	modified
internal/mycli/metrics/execution_metrics.go	91.1%	+4.4%	affected

---

Reported by octocov

[gemini-code-assist]

Code Review

This pull request provides excellent improvements by fixing several edge cases and optimizing performance. The changes correctly address a panic risk with short rows and a bug that prevented the full screen width from being utilized. The refactoring to reduce allocations and improve readability by replacing complex iterator chains with direct slice manipulation is a significant enhancement. I've identified one additional critical edge case where an empty headers slice could cause a panic in GreedyFrequencyStrategy and have left a comment with a suggested fix, aligning with our rule on defensive programming for edge cases.

[apstndb]

/gemini review

[gemini-code-assist]

Code Review

This pull request refactors the column width calculation strategies to fix several issues. It introduces a bounds check in GreedyFrequencyStrategy to prevent a panic when processing rows shorter than the header. It also modifies both GreedyFrequencyStrategy and ProportionalStrategy to ensure that any remaining available screen width is always distributed among columns, fixing a bug where it could be lost. The changes also include performance optimizations, such as reducing allocations by replacing iterator chains with direct slice manipulation and avoiding repeated calculations in adjustToSum. New tests have been added to cover the fixed edge cases.

[apstndb]

/gemini summary

[gemini-code-assist]

Summary of Changes

This pull request significantly refines the table formatting logic, focusing on robustness and efficiency. It resolves critical edge cases that could lead to panics, ensures that available screen width is always fully utilized across different width calculation strategies, and introduces performance improvements by reducing memory allocations. These changes contribute to a more stable and optimized display of tabular data within the application.

Highlights

• GreedyFrequencyStrategy Enhancements: Prevented panics with empty headers and short rows, optimized transposedRows creation to reduce allocations, and ensured full width utilization by always distributing remaining space.
• ProportionalStrategy Refinement: Modified the distribution logic to correctly assign remaining width even when all columns initially fit.
• Width Calculation Optimizations: Improved adjustToSum performance by pre-calculating unique thresholds and removed an unused helper function.
• Comprehensive Testing: Introduced new tests to cover edge cases for short rows, zero deficit, and full width utilization in both greedy and proportional strategies.

Changelog

• internal/mycli/format/width.go
  • Removed the unused clipToMax function.
  • Optimized the adjustToSum function to compute sorted unique thresholds once and inline clipping and summing.
• internal/mycli/format/width_strategy_greedy.go
  • Removed the math import as it was no longer needed.
  • Added a guard for empty headers to prevent panics.
  • Introduced an inline sumWidths function to replace lo.Sum for better performance and to avoid repeated closure evaluations.
  • Refactored the transposedRows creation to use direct slice construction, improving performance and preventing panics with short rows.
  • Modified the logic for distributing remaining width to always fall back to column 0 if no specific shortage is found, ensuring full width utilization.
• internal/mycli/format/width_strategy_proportional.go
  • Restructured the distribution block to ensure remaining width is assigned even when totalDeficit is zero, guaranteeing full available width usage.
• internal/mycli/format/width_strategy_test.go
  • Added TestGreedyFrequencyStrategy_ShortRows to verify handling of rows with fewer columns than headers.
  • Added TestProportionalStrategy_ZeroDeficit to ensure the proportional strategy uses full width when columns already fit.
  • Added TestGreedyFrequencyStrategy_FullWidth to ensure the greedy strategy uses full width when columns already fit.

Activity

• A bot provided an initial summary of changes and highlights.
• A bot posted a code metrics report, showing a slight increase in overall coverage and a significant reduction in test execution time.
• The author requested a review from the bot.
• The author requested a summary from the bot.
• A bot identified a critical issue in width_strategy_greedy.go regarding potential panics with empty headers, recommending a guard.
• The author acknowledged and confirmed the fix for the critical issue.