Conversation
seletz
left a comment
There was a problem hiding this comment.
Code Review
Critical
1. Spurious --merged flag on install subcommand
cmd/config/install.go:40
The install subcommand registers a --merged flag but never reads it — this is a copy-paste from config.go. Running odoo-work-cli config install --merged silently accepts the flag and does nothing. Delete line 40.
2. os.Exit in PersistentPreRunE bypasses cleanup and cobra error handling
cmd/odoo-work-cli/main.go:38-44
PersistentPreRunE calls fmt.Println(err); os.Exit(1) instead of returning the error. This bypasses PersistentPostRun (leaking partially-initialized connections) and breaks cobra's error handling/test infrastructure. Should just return err for both the config-load and client-creation failures.
Important
3. internal/parsing imports internal/tui — wrong dependency direction
internal/parsing/times.go:7
parsing is a utility package but imports the heavy internal/tui package just for ParseWeekMonday. This pure time-calculation function belongs in parsing (or a shared package), with tui importing it — not the other way around.
4. app.Deps holds concrete *odoo.XMLRPCClient instead of odoo.Client interface
internal/app/deps.go:13
The architecture is interface-driven (odoo.Client in internal/odoo/client.go), but Deps.Client uses the concrete type, making command packages untestable without a real XML-RPC server. Fix by adding Close() to the odoo.Client interface and typing Deps.Client as odoo.Client.
5. Wrong package name in cmd/config/
cmd/config/config.go:1, cmd/config/install.go:1
Both files declare package project but live in cmd/config/. All other command packages use their directory name (package clock, package entries, etc.). Should be package config.
6. Package-level flag variables in cmd/timesheet and cmd/tui
cmd/timesheet/timesheet.go:11, cmd/tui/tui.go:10
tsWeek and tuiWeek are package-level vars for cobra flags. Other refactored packages correctly scope flag state inside CMD(). These should follow the same pattern to avoid state leaking across test runs.
|
@StefanBethge could you please look into these? |
|
@seletz i'll Do it tomorrow. Maybe you can give me access to the Claude Review Tool. |
Cool, no stress.
Well I simply checked out your PR and told Claude Code to do a code review. It knows how to do that. There's no integration in workflow or something-- I just told it to do a code review, looked at it locally, seemed legit. Then I told it to add these comments as my review and ask for changes ... 🤷♂️ |
|
@seletz now all Code Review issues, should be fixed |
…d` initialization.
…`rootCmd` initialization, and modularize parsing and filtering logic.
…ing them into dedicated packages. Simplify `rootCmd` setup and reconfigure persistent pre-run logic.
…ges for entries, filter, and parsing.
…ncy injection and simplify client and config handling.
… and tests for verification. Remove unused `PersistentPreRunE` in the config command.
…ring unique identification for rows with the same project and task names across different companies. Refactor logic for row and hint key generation, improve sorting, and update tests for validation.
…functions for label wrapping and improve the rendering logic to ensure proper alignment. Add tests to validate label wrapping behavior.
…s, and totals. Add tests to verify row highlighting behavior.
…t/config handling, add dependency tests, and relocate `ParseWeekMonday` to `parsing` package. All fixes are responses for claude code review
79462e0 to
d024659
Compare
Summary
This PR improves the TUI in three areas:
Changes
TUI visual improvements
Closes #36
Company-aware grouping and search
This fixes cases where entries like Intern / Meeting from different companies could previously collapse into a single row or become ambiguous in search.
Closes #37
Main/bootstrap cleanup
Why
Before this change, the TUI had two usability problems:
This PR makes the grid easier to work with and fixes the underlying identity problem instead of only changing colors or presentation.
Closes #38