cal/major-domo-v2
1
0
Fork 0
Code Issues 6 Pull Requests 2 Actions Packages Projects Releases Wiki Activity

fix: update chart_service path from data/ to storage/ #119

Merged
cal merged 1 commits from fix/chart-service-storage-path into main 2026-03-21 02:01:56 +00:00
Conversation 0 Commits 1 Files Changed 1 +31 -33
cal commented 2026-03-21 01:29:17 +00:00
Owner
Copy Link

Summary

PR #86 moved state files from data/ to storage/ but missed chart_service.py. The /charts command autocomplete shows no results because the code reads from /app/data/charts.json while the file is at /app/storage/charts.json.

Changes

  • services/chart_service.py: CHARTS_FILE path changed from data/charts.json to storage/charts.json

Testing

One-line path change. Verified no other data/ references remain that should be storage/.

🤖 Generated with Claude Code

## Summary PR #86 moved state files from `data/` to `storage/` but missed `chart_service.py`. The `/charts` command autocomplete shows no results because the code reads from `/app/data/charts.json` while the file is at `/app/storage/charts.json`. ## Changes - `services/chart_service.py`: `CHARTS_FILE` path changed from `data/charts.json` to `storage/charts.json` ## Testing One-line path change. Verified no other `data/` references remain that should be `storage/`. 🤖 Generated with [Claude Code](https://claude.com/claude-code)
cal added 1 commit 2026-03-21 01:29:18 +00:00 
PR #86 moved state files to storage/ but missed chart_service.py,
which still pointed to data/charts.json. The file exists at
/app/storage/charts.json in the container but the code looked
in /app/data/charts.json, causing empty autocomplete results.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
cal reviewed 2026-03-21 01:30:22 +00:00
cal left a comment
Author
Owner
Copy Link

AI Code Review

Files Reviewed

  • services/chart_service.py (modified)

Findings

Correctness

  • The path fix is correct. CHARTS_FILE is now Path(__file__).parent.parent / "storage" / "charts.json", which resolves to /app/storage/charts.json in the container — matching the rw volume mount introduced in PR #86.
  • _save_charts() calls self.CHARTS_FILE.parent.mkdir(parents=True, exist_ok=True), so the storage/ directory is created automatically on first write if the volume mount is absent (consistent with the pattern used by the five other trackers updated in PR #86).
  • PR body claims "no other data/ references remain that should be storage/" — verified this is accurate. The only remaining data/ references in .py files are:
    • config.py:93 — sheets_credentials_path: str = "/app/data/major-domo-service-creds.json"
    • tests/test_services_draft_sheet.py:39 — sets config.sheets_credentials_path = "/app/data/test-creds.json"
    • Both are for the Google Sheets credentials file, which is intentionally kept on the separate :ro credential mount at /app/data/ (not a state file, correctly excluded from this migration).

Security

  • No issues found.

Style & Conventions

  • The linter also reformatted the file (single quotes → double quotes, trailing commas, list comprehension style, function signatures). These are cosmetic and consistent with the project's style.
  • One stale inline comment at line 86: # Ensure data directory exists should read # Ensure storage directory exists now that the path has changed. Minor — does not affect behaviour.

Suggestions

  • Update the comment at line 86 from # Ensure data directory exists to # Ensure storage directory exists to stay in sync with the new path. Trivial but keeps the code self-consistent.

Verdict: COMMENT

The path fix is correct and complete. The PR body's claim that no other data/ paths were missed has been independently verified. The one stale comment is cosmetic and does not block merging.

Automated review by Claude PR Reviewer

## AI Code Review ### Files Reviewed - `services/chart_service.py` (modified) ### Findings #### Correctness - The path fix is correct. `CHARTS_FILE` is now `Path(__file__).parent.parent / "storage" / "charts.json"`, which resolves to `/app/storage/charts.json` in the container — matching the `rw` volume mount introduced in PR #86. - `_save_charts()` calls `self.CHARTS_FILE.parent.mkdir(parents=True, exist_ok=True)`, so the `storage/` directory is created automatically on first write if the volume mount is absent (consistent with the pattern used by the five other trackers updated in PR #86). - PR body claims "no other `data/` references remain that should be `storage/`" — verified this is accurate. The only remaining `data/` references in `.py` files are: - `config.py:93` — `sheets_credentials_path: str = "/app/data/major-domo-service-creds.json"` - `tests/test_services_draft_sheet.py:39` — sets `config.sheets_credentials_path = "/app/data/test-creds.json"` - Both are for the Google Sheets credentials file, which is intentionally kept on the separate `:ro` credential mount at `/app/data/` (not a state file, correctly excluded from this migration). #### Security - No issues found. #### Style & Conventions - The linter also reformatted the file (single quotes → double quotes, trailing commas, list comprehension style, function signatures). These are cosmetic and consistent with the project's style. - One stale inline comment at line 86: `# Ensure data directory exists` should read `# Ensure storage directory exists` now that the path has changed. Minor — does not affect behaviour. #### Suggestions - Update the comment at line 86 from `# Ensure data directory exists` to `# Ensure storage directory exists` to stay in sync with the new path. Trivial but keeps the code self-consistent. ### Verdict: COMMENT The path fix is correct and complete. The PR body's claim that no other `data/` paths were missed has been independently verified. The one stale comment is cosmetic and does not block merging. --- *Automated review by Claude PR Reviewer*
cal force-pushed fix/chart-service-storage-path from f024757e2e to 3c453c89ce 2026-03-21 01:30:46 +00:00 Compare
Claude added the
ai-reviewing
 label 2026-03-21 01:45:48 +00:00
Claude approved these changes 2026-03-21 01:46:40 +00:00
Claude left a comment
Collaborator
Copy Link

AI Code Review

Files Reviewed

  • services/chart_service.py (modified)

Findings

Correctness

  • Path fix is correct. CHARTS_FILE updated from 'data' / 'charts.json' → "storage" / "charts.json", consistent with the storage/ volume introduced in PR #86.
  • _save_charts() already calls self.CHARTS_FILE.parent.mkdir(parents=True, exist_ok=True), so the storage/ directory is created automatically even on a fresh container — no deployment risk.
  • All other data/ references in the codebase are for the credentials file (major-domo-service-creds.json) which is intentionally still at data/ per PR #86's file-level credential mount design.
  • The remaining changes are cosmetic linter reformatting (single → double quotes, trailing commas, inline comprehensions) — no behavioral changes.

Security

  • No issues found.

Style & Conventions

  • Double-quote style and trailing commas are consistent with the ruff-enforced project style seen throughout recent PRs.

Suggestions

  • commands/utilities/CLAUDE.md still references data/charts.json at lines 166 and 197 — stale doc from before PR #86. Worth a follow-up cleanup alongside the commands/gameplay/CLAUDE.md and commands/voice/CLAUDE.md doc debt already noted from PR #86.

Verdict: APPROVED

One-line bug fix with correct mechanics. The storage directory is auto-created, no migration needed. Cosmetic linter changes are noise-free.

Automated review by Claude PR Reviewer

## AI Code Review ### Files Reviewed - `services/chart_service.py` (modified) ### Findings #### Correctness - **Path fix is correct.** `CHARTS_FILE` updated from `'data' / 'charts.json'` → `"storage" / "charts.json"`, consistent with the `storage/` volume introduced in PR #86. - `_save_charts()` already calls `self.CHARTS_FILE.parent.mkdir(parents=True, exist_ok=True)`, so the `storage/` directory is created automatically even on a fresh container — no deployment risk. - All other `data/` references in the codebase are for the credentials file (`major-domo-service-creds.json`) which is intentionally still at `data/` per PR #86's file-level credential mount design. - The remaining changes are cosmetic linter reformatting (single → double quotes, trailing commas, inline comprehensions) — no behavioral changes. #### Security - No issues found. #### Style & Conventions - Double-quote style and trailing commas are consistent with the ruff-enforced project style seen throughout recent PRs. #### Suggestions - `commands/utilities/CLAUDE.md` still references `data/charts.json` at lines 166 and 197 — stale doc from before PR #86. Worth a follow-up cleanup alongside the `commands/gameplay/CLAUDE.md` and `commands/voice/CLAUDE.md` doc debt already noted from PR #86. ### Verdict: APPROVED One-line bug fix with correct mechanics. The storage directory is auto-created, no migration needed. Cosmetic linter changes are noise-free. --- *Automated review by Claude PR Reviewer*
Claude added
ai-reviewed
 and removed
ai-reviewing
 labels 2026-03-21 01:47:01 +00:00
cal merged commit 6889499fff into main 2026-03-21 02:01:56 +00:00
cal deleted branch fix/chart-service-storage-path 2026-03-21 02:01:56 +00:00
Sign in to join this conversation.
Reviewers
No reviewers
Claude
Labels
Clear labels   
ai-changes-requested

   
ai-pr-opened

   
ai-reviewed

   
ai-reviewing

   
ai-working

   
in-next-release

Fix included in the next-release branch, pending merge to main

  
status/in-progress

Work is actively in progress

  
status/pr-open

A pull request has been opened for this issue

No Label
ai-changes-requested
ai-pr-opened
ai-reviewed
ai-reviewing
ai-working
in-next-release
status/in-progress
status/pr-open
Milestone
Clear milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
No Assignees
2 Participants
Notifications
Due Date
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: cal/major-domo-v2#119
No description provided.
Delete Branch "fix/chart-service-storage-path"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?