Code cleanup before v1 - #12
Merged
Merged
Conversation
|
Caution Review failedPull request was closed or merged during review WalkthroughFixes OAuth callback to a fixed localhost port, introduces TokenSet parsing/storage, centralizes atomic file writes in a persistence module, exposes crate-scoped schema constructor, adds config Changes
Sequence Diagram(s)sequenceDiagram
participant User as User (CLI)
participant Browser as Browser
participant OAuth as OAuth Server
participant Callback as Local Callback (127.0.0.1:19876)
participant TokenAPI as Token Endpoint
participant Config as Config Store
participant FS as Persistence (write_atomic)
User->>Browser: open auth URL (build_auth_url)
Browser->>OAuth: authenticate & authorize
OAuth->>Callback: redirect with code
Callback->>User: deliver code to CLI flow
User->>TokenAPI: exchange code for tokens
TokenAPI-->>User: return token JSON
User->>Config: TokenSet::from_json & store_tokens
Config->>FS: write_atomic(config.toml)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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: 4
🧹 Nitpick comments (2)
src/api.rs (1)
665-667: Resolve organization ID once per operation fill pass.If both required path and query
organization_idare present, this can callresolve_organizationmore than once. Cache it locally to avoid repeated resolution.♻️ Proposed refactor
fn fill_defaults_from_config( operation: &Operation, client: &HubstaffClient, organization_override: Option<u64>, path_values: &mut HashMap<String, String>, query_values: &mut HashMap<String, String>, ) -> Result<(), CliError> { + let mut resolved_organization_id: Option<String> = None; + for parameter in operation .parameters .iter() .filter(|parameter| parameter.required) { @@ - let organization_id = client - .resolve_organization(organization_override)? - .to_string(); + let organization_id = if let Some(value) = &resolved_organization_id { + value.clone() + } else { + let value = client.resolve_organization(organization_override)?.to_string(); + resolved_organization_id = Some(value.clone()); + value + };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/api.rs` around lines 665 - 667, The code calls client.resolve_organization(organization_override) multiple times when both path and query organization_id are present; change the logic to call client.resolve_organization once per operation fill pass, store its result in a local variable (e.g., organization_id from resolve_organization) and reuse that cached string wherever organization resolution is needed (referencing resolve_organization and organization_id) so the resolution is not performed repeatedly.justfile (1)
39-39: Consider hardening schema fixture download for transient network failures.Adding retry/timeout flags will make this maintenance command less flaky.
🛠️ Suggested tweak
- curl -sSf https://api.hubstaff.com/v2/docs -o tests/fixtures/schema.json + curl -sSf --retry 3 --retry-all-errors --connect-timeout 10 --max-time 60 \ + https://api.hubstaff.com/v2/docs -o tests/fixtures/schema.json🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@justfile` at line 39, Update the curl invocation "curl -sSf https://api.hubstaff.com/v2/docs -o tests/fixtures/schema.json" to be resilient to transient network failures by adding retry and timeout flags: configure a finite retry count (e.g. 2–5 retries), a short retry delay, enable retry on connection refused, and set an overall max-time/timeout so the command fails fast on long hangs; keep existing silent/fail behavior and still write to tests/fixtures/schema.json.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/auth.rs`:
- Around line 10-11: The redirect URI constant and the local HTTP listener
disagree on host (CALLBACK_REDIRECT_URI uses "localhost" while the server binds
to "127.0.0.1"), causing missed callbacks when "localhost" resolves to ::1;
update the code so both sides use the same host: either change
CALLBACK_REDIRECT_URI to "http://127.0.0.1:19876/callback" (preferred) or modify
the listener that currently binds to "127.0.0.1" to bind to both IPv4/IPv6
loopback (e.g., "::1" or both addresses) so CALLBACK_PORT and
CALLBACK_REDIRECT_URI remain consistent; apply the same change to the other
occurrences referenced around the server binding (the code that constructs the
listener and uses CALLBACK_PORT/CALLBACK_REDIRECT_URI).
In `@src/config.rs`:
- Around line 123-128: The store_tokens method currently retains the old
auth.expires_at when the incoming TokenSet has expires_at == None; update the
logic in pub fn store_tokens(&mut self, tokens: TokenSet) so that if
tokens.expires_at is Some(value) you set self.auth.expires_at = Some(value),
otherwise you explicitly clear self.auth.expires_at = None to avoid carrying
stale expiry metadata from a previous token; reference store_tokens,
TokenSet::expires_at, and self.auth.expires_at to locate and change the
assignment.
In `@src/persistence.rs`:
- Around line 6-15: The current code uses a fixed sibling temp path
(path.with_extension("tmp")) which can collide across concurrent writers; change
the temp file creation to use a unique per-write temporary filename in the same
directory (e.g., include a UUID/timestamp+PID/random suffix or use a temp file
API that creates a NamedTempFile in the target dir) instead of a fixed ".tmp"
name, write bytes to that unique temp_path, set permissions via the existing
unix block (fs::set_permissions / PermissionsExt) and then atomically fs::rename
the unique temp_path to path; update references to temp_path, fs::write, and
fs::rename accordingly so each write uses its own temp file.
In `@tests/commands_test.rs`:
- Around line 217-218: Replace the fixed "/tmp/hcli-test-*" paths with per-test
temporary directories (e.g., using tempfile::TempDir or tempfile::Builder to
create a unique dir) and update uses of the dir variable accordingly;
specifically change the `let dir = "/tmp/hcli-test-set-token-clears-refresh";`
(and the other occurrences at the mentioned locations) to create a TempDir, use
its path() for subsequent filesystem operations, and remove the manual
std::fs::remove_dir_all call (or scope cleanup) so tests no longer rely on
deterministic global paths and are safe for parallel runs and aborted runs.
---
Nitpick comments:
In `@justfile`:
- Line 39: Update the curl invocation "curl -sSf
https://api.hubstaff.com/v2/docs -o tests/fixtures/schema.json" to be resilient
to transient network failures by adding retry and timeout flags: configure a
finite retry count (e.g. 2–5 retries), a short retry delay, enable retry on
connection refused, and set an overall max-time/timeout so the command fails
fast on long hangs; keep existing silent/fail behavior and still write to
tests/fixtures/schema.json.
In `@src/api.rs`:
- Around line 665-667: The code calls
client.resolve_organization(organization_override) multiple times when both path
and query organization_id are present; change the logic to call
client.resolve_organization once per operation fill pass, store its result in a
local variable (e.g., organization_id from resolve_organization) and reuse that
cached string wherever organization resolution is needed (referencing
resolve_organization and organization_id) so the resolution is not performed
repeatedly.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b80b30cc-28c0-4e0c-94a0-359867522473
⛔ Files ignored due to path filters (2)
Cargo.lockis excluded by!**/*.locksrc/snapshots/hubstaff__command_index__tests__schema_command_table_snapshot.snapis excluded by!**/*.snap
📒 Files selected for processing (14)
Cargo.tomlREADME.mdjustfilesrc/api.rssrc/auth.rssrc/command_index.rssrc/config.rssrc/config_commands.rssrc/login.rssrc/main.rssrc/persistence.rssrc/schema.rstests/commands_test.rstests/fixtures/schema.json
💤 Files with no reviewable changes (1)
- src/login.rs
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/auth.rs`:
- Around line 145-156: In from_json, do not call chrono::Duration::seconds() or
use direct DateTime + Duration arithmetic on the raw expires_in because those
can panic on out-of-range values; instead parse expires_in with as_i64(),
convert to a Duration using chrono::Duration::try_seconds(secs) and then use
chrono::Utc::now().checked_add_signed(duration) to produce expires_at, and if
either try_seconds or checked_add_signed returns None or the field is missing,
return Err(CliError::Auth(...)) so malformed/too-large expires_in values produce
an error rather than a panic.
In `@src/config.rs`:
- Around line 154-175: When the "auth_url" branch updates self.auth_url (the
match arm that sets DEFAULT_AUTH_URL) you must also clear any preserved
authentication to avoid using stale refresh tokens; update that branch to
reset/clear self.auth when changing auth_url. Likewise, modify pub fn reset(&mut
self) so it does not keep the prior auth: instead of preserving auth via let
auth = std::mem::take(&mut self.auth) and reusing it, set self.auth to its
default/empty state (or otherwise couple tokens to the endpoint) so reset()
clears credentials along with other fields.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 4f9c6ca6-9239-4c74-ac6e-53f821c10a77
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (7)
Cargo.tomlREADME.mdsrc/auth.rssrc/config.rssrc/config_commands.rssrc/persistence.rstests/commands_test.rs
✅ Files skipped from review due to trivial changes (1)
- Cargo.toml
🚧 Files skipped from review as they are similar to previous changes (3)
- README.md
- src/persistence.rs
- tests/commands_test.rs
ayarotsky
force-pushed
the
chore_code_clean_up
branch
from
April 21, 2026 19:43
77f5044 to
9426a1b
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
config unsetandconfig resetcommands--organizationCLI option to override organization per requestBug Fixes / Reliability
Documentation
Tests