Global use and change proyects#7
Conversation
…improved performance
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request transitions the application to a global SQLite-backed architecture, introducing multi-project support and an incremental balance caching system. The user interface has been modernized using the rich library, and data storage now adheres to XDG standards. Key feedback includes resolving a NameError for the missing migrate_from_json function, addressing SQLite connection leaks in the storage and CLI modules, and optimizing the database schema by including the total_balance column in the initial table creation. Improvements to the KeyboardInterrupt handling are also recommended to ensure a smoother exit flow for users.
There was a problem hiding this comment.
Pull request overview
This PR modernizes time-balance into a global, multi-project CLI backed by centralized SQLite storage, adds an incremental cached-balance model for performance, and refreshes user/developer documentation and tests to match the new architecture.
Changes:
- Replaced legacy JSON persistence with a SQLite-based
DatabaseManagerplus cachedtotal_balanceupdates. - Redesigned the CLI UX using
rich(dashboard, paginated history, config/project submenus) and adjusted import/export flows. - Updated packaging/versioning (dynamic version file,
richdependency), documentation, and test suite (including new balance-cache tests).
Reviewed changes
Copilot reviewed 22 out of 22 changed files in this pull request and generated 17 comments.
Show a summary per file
| File | Description |
|---|---|
| time_balance/storage.py | Introduces SQLite DatabaseManager, cached balance logic, and legacy shims |
| time_balance/io.py | Refactors import/export utilities to validate/read/write legacy JSON files |
| time_balance/i18n.py | Updates and expands translations for new menus and navigation |
| time_balance/constants.py | Adds XDG-style global data dir resolution and DB path; version from file |
| time_balance/cli.py | New rich-based UI, pagination, config/project menus, new CLI args |
| time_balance/init.py | Updates public re-exports to reflect new modules/functions |
| time_balance/VERSION | Adds a single-source version file (0.4.1) |
| tests/test_storage.py | Reworks tests around SQLite-based storage manager |
| tests/test_io.py | Updates tests for new export_history / read_history_file behavior |
| tests/test_cli.py | Updates CLI tests to patch DB and capture rich output |
| tests/test_balance_cache.py | Adds focused tests for incremental cache correctness |
| setup.py | Reads version from file; adds rich dependency and package data |
| pyproject.toml | Uses dynamic version from file; declares dependencies and package data |
| docs/es/DEVELOPMENT.es.md | Updates Spanish dev guide for new architecture |
| docs/es/CLI-GUIDE.es.md | Updates Spanish CLI guide for global/multi-project workflow |
| docs/es/ARCHITECTURE.es.md | Updates Spanish architecture doc for SQLite/caching design |
| docs/DEVELOPMENT.md | Updates dev guide for new layering and SQLite persistence |
| docs/CLI-GUIDE.md | Updates CLI guide for new navigation, global usage, and commands |
| docs/ARCHITECTURE.md | Updates architecture doc for SQLite schema and caching strategy |
| README.md | Updates English README for global/multi-project SQLite-based tool |
| README.es.md | Updates Spanish README for global/multi-project SQLite-based tool |
| CHANGELOG.md | Documents new releases/features (0.3.0 / 0.4.1) |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| # Display current configuration | ||
| console.print(f"\n [dim]{translate('project_label', lang=lang)}:[/dim] [bold]{project['name']}[/bold]") | ||
| console.print(f" [dim]{translate('base_day_label', lang=lang)}:[/dim] [bold]{project['base_hours']}h {project['base_minutes']}m[/bold]") | ||
| console.print(f" [dim]Language:[/dim] [bold]{db.get_setting('language', 'auto')}[/bold]") |
There was a problem hiding this comment.
The config menu prints Language: as a hard-coded English string, bypassing the i18n layer used elsewhere. Add a translation key (e.g., language_label) and use translate(...) so the menu stays fully localized.
| console.print(f" [dim]Language:[/dim] [bold]{db.get_setting('language', 'auto')}[/bold]") | |
| console.print(f" [dim]{translate('language_label', lang=lang)}:[/dim] [bold]{db.get_setting('language', 'auto')}[/bold]") |
| - **Global Installation**: The app now stores data in standard system paths (XDG compliant), making it truly global. | ||
| - **SQLite Backend**: Replaced JSON storage with SQLite for improved performance and data integrity. | ||
| - **Multi-project Support**: New project management menu allows creating, switching, and editing multiple work contexts. | ||
| - **Migration Tool**: New `--migrate <file.json>` command to import legacy history files into the new system. |
There was a problem hiding this comment.
Changelog claims a working --migrate <file.json> command exists, but cli.py currently references an undefined migrate_from_json function, so the flag will fail at runtime. Either implement the migration command or adjust the changelog entry until it’s actually available.
| - **Migration Tool**: New `--migrate <file.json>` command to import legacy history files into the new system. | |
| - **Migration Preparation**: Added groundwork for importing legacy JSON history files into the new system. |
| if args.migrate: | ||
| migrate_from_json(args.migrate, lang=lang) | ||
| return |
There was a problem hiding this comment.
--migrate will currently crash with NameError because migrate_from_json is referenced but not implemented/imported anywhere. Either implement migrate_from_json in this module (and add tests), or remove the CLI flag/call until the migration flow is ready.
| if mode == constants.MODE_OVERWRITE: | ||
| with db._get_connection() as conn: | ||
| conn.execute("DELETE FROM records WHERE project_id = ?", (active_id,)) | ||
|
|
There was a problem hiding this comment.
This flow reaches into the DB manager’s private _get_connection() to delete records, which makes the CLI depend on internal implementation details. Expose a public method on DatabaseManager for “clear project records” / “delete all records for project” and call that instead.
| with self._get_connection() as connection: | ||
| cursor = connection.cursor() | ||
|
|
||
| # 1. Get old difference if record exists to adjust balance | ||
| cursor.execute("SELECT difference FROM records WHERE project_id = ? AND date = ?", (project_id, record_date)) | ||
| row = cursor.fetchone() | ||
| old_difference = row[0] if row else 0 | ||
|
|
||
| # 2. Upsert the record | ||
| cursor.execute(""" | ||
| INSERT OR REPLACE INTO records (project_id, date, hours, minutes, difference) | ||
| VALUES (?, ?, ?, ?, ?) | ||
| """, (project_id, record_date, hours, minutes, difference)) |
There was a problem hiding this comment.
upsert_record reads old_difference and then writes using a separate statement, but the initial SELECT can occur outside a transaction, so another writer could change the record between the read and the write (breaking the “atomic balance update” guarantee). Start the transaction before reading (e.g., BEGIN IMMEDIATE) and/or use an INSERT ... ON CONFLICT DO UPDATE so the upsert happens without REPLACE semantics (which also resets id/created_at).
| choices.append("p") | ||
|
|
||
| console.print(nav_msg) | ||
| choice = Prompt.ask("", choices=choices, show_choices=False, console=console).lower() |
There was a problem hiding this comment.
This prompt advertises navigation keys as uppercase (V/N/P) but Prompt.ask(..., choices=...) is configured with lowercase-only choices and defaults to case-sensitive validation in Rich. Either set case_sensitive=False or include uppercase variants in choices so users can type the keys as displayed.
| choice = Prompt.ask("", choices=choices, show_choices=False, console=console).lower() | |
| choice = Prompt.ask("", choices=choices, show_choices=False, console=console, case_sensitive=False).lower() |
| # Guía para Desarrolladores | ||
|
|
||
| Esta guía proporciona detalles técnicos para desarrolladores que deseen integrar `time-balance` en sus flujos o contribuir a su desarrollo. | ||
| Esta guía proporciona detalles técnicos para desarrolladores que deseen contribuir a la versión 0.3.x de `time-balance`. |
There was a problem hiding this comment.
Esta guía indica que las contribuciones son para la “versión 0.3.x”, pero el proyecto ya está en 0.4.1. Actualiza esa referencia para que coincida con la versión real (y con time_balance/VERSION) y evitar confusión.
| Esta guía proporciona detalles técnicos para desarrolladores que deseen contribuir a la versión 0.3.x de `time-balance`. | |
| Esta guía proporciona detalles técnicos para desarrolladores que deseen contribuir a la versión 0.4.1 de `time-balance`. |
| # Migrar un archivo JSON de legado a un nuevo proyecto global | ||
| time-balance --migrate ./ruta/al/historial.json |
There was a problem hiding this comment.
Esta guía documenta --migrate, pero en el código actual (cli.py) se llama a una función inexistente (migrate_from_json), por lo que el flag fallará. O bien se implementa la migración, o se elimina/etiqueta como “pendiente” hasta que funcione.
| # import_history was previously handling backups and saving. | ||
| # Now cli.py will handle database interaction. | ||
| # We keep a simpler version or remove it if cli.py handles everything. |
There was a problem hiding this comment.
import_history appears to have been removed entirely (only a comment remains), which is another breaking change for anyone using the library API. If this removal is intentional, consider providing a deprecation period or a replacement helper that performs the DB import so external callers don’t lose functionality abruptly.
…oved error handling and connection management
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 22 out of 22 changed files in this pull request and generated 16 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| import os | ||
| import argparse | ||
| from datetime import date, datetime | ||
| from typing import Optional |
There was a problem hiding this comment.
cli.py imports os and Optional but neither is used anymore. This will trip linters and makes the module look like it still has OS-dependent behavior.
| import os | |
| import argparse | |
| from datetime import date, datetime | |
| from typing import Optional | |
| import argparse | |
| from datetime import date, datetime |
| lang_opt = Prompt.ask( | ||
| translate("language_prompt", lang=lang, current=db.get_setting("language", "auto")), | ||
| choices=["en", "es", "auto"], | ||
| default="auto", |
There was a problem hiding this comment.
The language prompt in config_menu hard-codes default="auto". If the current setting is en/es, pressing Enter will unexpectedly reset the language to auto. The default should typically be the current value from settings so Enter keeps the existing selection.
| lang_opt = Prompt.ask( | |
| translate("language_prompt", lang=lang, current=db.get_setting("language", "auto")), | |
| choices=["en", "es", "auto"], | |
| default="auto", | |
| current_language = db.get_setting("language", "auto") | |
| lang_opt = Prompt.ask( | |
| translate("language_prompt", lang=lang, current=current_language), | |
| choices=["en", "es", "auto"], | |
| default=current_language, |
| ## [0.4.2] - 2026-04-24 | ||
|
|
||
| ### Fixed | ||
| - **NameError in Migration**: Fixed missing `migrate_from_json` function reported in code review. | ||
| - **SQLite Connection Leaks**: Implemented a robust context manager in `DatabaseManager` to ensure all database connections are properly closed after use. |
There was a problem hiding this comment.
Changelog adds a 0.4.2 section, but the package version is read from time_balance/VERSION (currently 0.4.1), so --version and packaging metadata won’t match the changelog. Either bump time_balance/VERSION to 0.4.2 or adjust the changelog heading to the released version.
| try: | ||
| h = int(Prompt.ask(translate("base_hours_prompt", lang=lang, current=constants.BASE_HOURS), default=str(constants.BASE_HOURS), console=console)) | ||
| m = int(Prompt.ask(translate("base_minutes_prompt", lang=lang, current=constants.BASE_MINUTES), default=str(constants.BASE_MINUTES), console=console)) | ||
| new_id = db.create_project(name, h, m) | ||
| db.set_active_project_id(new_id) |
There was a problem hiding this comment.
Project creation only catches ValueError. If the chosen project name already exists, db.create_project() will raise sqlite3.IntegrityError (UNIQUE constraint) and the CLI will crash. Consider catching sqlite3.IntegrityError here and prompting for a different name.
| - Support for history merging (`merge`) or total replacement (`overwrite`). | ||
| Logic to validate and read external JSON files. | ||
| - `read_history_file()`: Validates schemas from previous versions (v0.2.x). | ||
| - `export_history()`: Generates a JSON dump of the active project. |
There was a problem hiding this comment.
export_history() no longer “generates a JSON dump of the active project” by itself; it simply writes the provided data dict to disk (the CLI constructs the dump). Updating this description will help contributors understand where the export payload is assembled.
| - `export_history()`: Generates a JSON dump of the active project. | |
| - `export_history()`: Writes a provided project-history data dict to a JSON file; the CLI assembles the export payload. |
| # --- GLOBAL SINGLETON --- | ||
| db = DatabaseManager(constants.DATABASE_PATH) | ||
|
|
There was a problem hiding this comment.
The global singleton db = DatabaseManager(constants.DATABASE_PATH) is created at import time, which will create directories and the real user database just by importing time_balance (including during unit tests). This has undesirable side effects and makes it hard to run tests without touching the user environment. Consider lazy-initializing the singleton (e.g., get_db()), or constructing it inside the CLI entrypoints and passing it through (with tests injecting a temp DB).
| project = db.get_project_by_id(project_id) | ||
| records_list = db.get_records(project_id) | ||
|
|
||
| # Convert list of records to dict by date | ||
| records_dict = {r['date']: {'hours': r['hours'], 'minutes': r['minutes'], 'difference': r['difference']} for r in records_list} | ||
|
|
There was a problem hiding this comment.
load_data() assumes the active project exists (project = db.get_project_by_id(project_id) then later indexes project[...]). If active_project_id is stale/corrupted, project can be None and this will crash. Consider handling None with a fallback/reseed or a clearer error.
| project = db.get_project_by_id(project_id) | |
| records_list = db.get_records(project_id) | |
| # Convert list of records to dict by date | |
| records_dict = {r['date']: {'hours': r['hours'], 'minutes': r['minutes'], 'difference': r['difference']} for r in records_list} | |
| if project_id is None: | |
| raise RuntimeError("No active project is configured.") | |
| project = db.get_project_by_id(project_id) | |
| if project is None: | |
| raise RuntimeError(f"Active project {project_id} does not exist.") | |
| records_list = db.get_records(project_id) | |
| # Convert list of records to dict by date | |
| records_dict = {r['date']: {'hours': r['hours'], 'minutes': r['minutes'], 'difference': r['difference']} for r in records_list} |
| """Shim for compatibility. Saves metadata to the active project.""" | ||
| if file_path: | ||
| # If a file_path is provided, we might be exporting or using a non-standard flow | ||
| # For now, we ignore it and warn, or implement as needed. | ||
| pass |
There was a problem hiding this comment.
save_data() shim silently ignores file_path (it just passes). Callers providing a file path may assume it’s respected, but it currently does nothing. Consider raising a clear NotImplementedError/warning or implementing the intended export/custom-path behavior to avoid silent misbehavior.
| base_dir = pathlib.Path.home() / "AppData" / "Roaming" | ||
| else: | ||
| base_dir = pathlib.Path(base_dir) | ||
| elif os.uname().sysname == 'Darwin': |
There was a problem hiding this comment.
get_data_dir() uses os.uname() to detect macOS. os.uname is not available on all Python platforms/environments, so this can raise AttributeError even when os.name != 'nt'. Consider using sys.platform == 'darwin' (or platform.system()) to avoid relying on os.uname.
| elif os.uname().sysname == 'Darwin': | |
| elif os.sys.platform == 'darwin': |
| query = "SELECT * FROM records WHERE project_id = ? ORDER BY date DESC" | ||
| parameters = [project_id] | ||
|
|
||
| if limit: |
There was a problem hiding this comment.
get_records() checks if limit: so limit=0 behaves like limit=None and returns all rows. If callers pass 0 expecting an empty result set, this is surprising. Prefer if limit is not None: (allowing LIMIT 0) or validate limit > 0.
| if limit: | |
| if limit is not None: |
This pull request updates the documentation and user guides to reflect major architectural and feature improvements in the latest versions of
time-balance, especially the transition to a global, multi-project, and high-performance caching system. The documentation now emphasizes centralized SQLite storage, atomic balance updates, improved CLI navigation, and a professional user experience.Key changes include:
Feature and Architecture Documentation
Updated
CHANGELOG.md,README.md,README.es.md, anddocs/ARCHITECTURE.mdto document new features: global installation, multi-project support, SQLite backend, high-performance balance caching, and atomic incremental balance updates. [1] [2] [3] [4] [5] [6] [7] [8]Expanded architecture documentation to describe the new database schema, project and record management, atomic update strategies, and the redesigned test suite. [1] [2]
CLI and User Experience
docs/CLI-GUIDE.md) to explain the new global, multi-project workflow, standard navigation keys, paginated history, and enhanced configuration and project management menus. [1] [2]Data Management and Location
Test and Reliability Improvements
Internationalization and Professionalism
These changes ensure that all user-facing and developer documentation accurately represents the current capabilities and design of
time-balance.