fix(notifications): resolve analytics DB path from config, not $HOME default#117
Conversation
|
Warning Review limit reached
More reviews will be available in 3 minutes and 6 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more credits in the billing tab to continue. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe ChangesNotifications DB Path Resolution
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related issues
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cmd/hookwise/cmd_notifications.go`:
- Around line 41-45: The os.Getwd() error is being ignored with a blank
identifier, allowing an empty cwd to be passed to core.LoadConfig when the
working directory cannot be determined. Instead of ignoring the os.Getwd()
error, check it explicitly and if os.Getwd() fails, directly assign config to
core.GetDefaultConfig() without attempting to load config with an invalid cwd.
This ensures that when getting the current working directory fails, the code
immediately falls back to defaults rather than silently proceeding with an empty
directory path that could result in pointing notifications to the wrong database
path.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 2e38555a-c868-46ca-a61b-492faae4d630
📒 Files selected for processing (2)
cmd/hookwise/cmd_notifications.gocmd/hookwise/cmd_notifications_test.go
| cwd, _ := os.Getwd() | ||
| config, err := core.LoadConfig(cwd) | ||
| if err != nil { | ||
| config = core.GetDefaultConfig() | ||
| } |
There was a problem hiding this comment.
Handle os.Getwd() failure explicitly before loading config.
If os.Getwd() fails, this code currently proceeds with an empty/invalid project dir and then silently falls back to defaults, which can point notifications to the wrong DB path despite a valid project config. Treat Getwd failure as a direct fallback branch.
Suggested patch
- cwd, _ := os.Getwd()
- config, err := core.LoadConfig(cwd)
- if err != nil {
- config = core.GetDefaultConfig()
- }
+ config := core.GetDefaultConfig()
+ if cwd, wdErr := os.Getwd(); wdErr == nil {
+ if loaded, err := core.LoadConfig(cwd); err == nil {
+ config = loaded
+ }
+ }
db, err := analytics.Open(resolveAnalyticsDBPath(dataDir, config))🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@cmd/hookwise/cmd_notifications.go` around lines 41 - 45, The os.Getwd() error
is being ignored with a blank identifier, allowing an empty cwd to be passed to
core.LoadConfig when the working directory cannot be determined. Instead of
ignoring the os.Getwd() error, check it explicitly and if os.Getwd() fails,
directly assign config to core.GetDefaultConfig() without attempting to load
config with an invalid cwd. This ensures that when getting the current working
directory fails, the code immediately falls back to defaults rather than
silently proceeding with an empty directory path that could result in pointing
notifications to the wrong database path.
`hookwise notifications` opened the analytics DB from only the --data-dir flag, falling through to DefaultDBPath() (~/.hookwise/analytics.db) and ignoring config.Analytics.DBPath. The writer (dispatch) records notifications to the config path, so under a custom analytics.db_path the reader opened the wrong (default/empty) DB and showed no history. Resolve the path the same way `stats` does (PR #107): LoadConfig(cwd) with a GetDefaultConfig fallback (ARCH-1 fail-open), then analytics.Open(resolveAnalyticsDBPath(dataDir, config)). Reader-side only — no dispatch hot path, no daemon coordination touched. Adds TestNotifications_ReadsConfigDBPath: with HOOKWISE_STATE_DIR isolated and a custom analytics.db_path configured, a notification seeded into the config DB is shown when --data-dir is empty (fails on the old code, passes on the fix). Part of the #109 reader/writer path-divergence class. The remaining two divergences (snapshots dir + TUI PID) touch the daemon writer and are tracked separately in #116 for a supervised PR. Refs #109 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
26772ad to
2b31d13
Compare
1 participant
What & why
hookwise notificationsis a reader that opened the analytics DB from only the--data-dirflag — falling through toanalytics.DefaultDBPath()(~/.hookwise/analytics.db) and ignoringconfig.Analytics.DBPath. The writer (dispatch) records notifications to the config path. So under a customanalytics.db_path, the reader opened the wrong (default/empty) DB and showed no history.This is the same reader/writer divergence class as the merged
stats(#107) anddoctor(#108) fixes — part of #109.Fix
Resolve the path exactly like
runStats:core.LoadConfig(cwd)with aGetDefaultConfig()fallback (ARCH-1 fail-open), thenanalytics.Open(resolveAnalyticsDBPath(dataDir, config)). Reuses the existingresolveAnalyticsDBPathhelper — no new resolver. Reader-side only; the dispatch hot path and daemon coordination are untouched.Test (TDD)
TestNotifications_ReadsConfigDBPath— withHOOKWISE_STATE_DIRpointed at an empty temp dir (so the default DB is absent) and a projecthookwise.yamlsetting a customanalytics.db_path, a notification seeded into the config DB is shown when--data-diris empty. RED→GREEN verified: stashing the production fix makes the test fail ("No notifications.") on the old code.Guarded gate green locally:
GOMEMLIMIT=4GiB go test -race -p 2 ./cmd/hookwise/→ok(0 zombies before/after).Scope
Fixes divergence #1 of three found by the #109 reader-path audit. The other two (
DefaultSnapshotsDir()+tuiPIDPath()) touch the daemon writer and are tracked in #116 for a supervised PR rather than autopatched here.Refs #109
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
--data-dirflag to the notifications command for overriding the default database directory.--limitflag to control the maximum number of notifications displayed.Tests