add global --color always/auto/never option, and session --days option#25
add global --color always/auto/never option, and session --days option#25smolin wants to merge 1 commit intoShlomob:mainfrom
Conversation
📝 WalkthroughWalkthroughAdded a Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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 Tip You can get early access to new features in CodeRabbit.Enable the |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8a627f28ba
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| elif session_end_date and TimeUtils.date_in_range( | ||
| session_end_date, start_date, end_date | ||
| ): |
There was a problem hiding this comment.
filter_sessions_by_date now keeps sessions when end_time falls in range, but downstream reports like generate_daily_report/generate_weekly_report bucket sessions by session.start_time.date(). That means a session that starts before the requested period and ends inside it is now included, then counted on an out-of-range day, so month/year-filtered outputs can show dates and totals outside the requested window.
Useful? React with 👍 / 👎.
| # Load all sessions first (without limit) | ||
| sessions_list = analyzer.analyze_all_sessions(path) |
There was a problem hiding this comment.
The sessions command now always calls analyze_all_sessions(path) and only applies --limit after loading everything in memory. This bypasses the existing loader/database limit path, so sessions --limit N regresses from bounded reads to full-history scans, which can significantly slow down or exhaust memory on large SQLite/file datasets.
Useful? React with 👍 / 👎.
![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
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@ocmonitor/cli.py`:
- Around line 318-344: The code always calls
SessionAnalyzer.analyze_all_sessions(path) and only slices in-memory, which
defeats loader-side limiting; change the call so when days is None you pass the
limit into analyzer.analyze_all_sessions(path, limit=limit) to let the loader
apply the limit, and keep the existing path where if days is specified you call
analyze_all_sessions(path) (no limit), then run
analyzer.filter_sessions_by_date(sessions_list, start_date, end_date) and only
after that apply sessions_list = sessions_list[:limit] if limit is set; update
references to analyze_all_sessions, filter_sessions_by_date, sessions_list,
limit, and days accordingly.
- Around line 205-213: The code mutates Console._color_system after construction
with an invalid value; instead construct Console using its public flags: when
color == "always" pass force_terminal=True; when color == "never" pass
no_color=True; otherwise leave defaults (or pass
force_terminal=False/no_color=False for "auto"). Update the Console(...) call
(the Console instantiation in ocmonitor/cli.py) to set these parameters based on
the color variable rather than mutating console._color_system.
- Around line 286-288: The --days option currently allows 0/negative values
which later make start_date = end_date - timedelta(days=days - 1) produce an
invalid/inverted range; change the click option type for "--days" (the option
that sets the variable days) from type=int to type=click.IntRange(min=1) so
invalid values are rejected at parse time, and ensure click.IntRange is imported
where the CLI is defined.
In `@ocmonitor/services/session_analyzer.py`:
- Around line 249-267: The current filter block requires session_start_date and
thus skips sessions that lack a start timestamp but have session_end_date within
the query window; update the logic in the session filtering block (where
session_start_date, session_end_date, TimeUtils.date_in_range, start_date,
end_date and filtered.append(session) are used) to treat start and end
independently: if session_start_date exists check if it falls in range (or spans
the range with session_end_date), and separately if session_end_date exists
check if it falls in range; set in_range true if either check passes so sessions
with only end_time are included.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 0c74f014-3956-47bb-804e-0a23cc2932f0
📒 Files selected for processing (2)
ocmonitor/cli.pyocmonitor/services/session_analyzer.py
| console = Console(theme=theme_obj) | ||
|
|
||
| # Apply --color setting | ||
| if color == "always": | ||
| console._color_system = "true" | ||
| elif color == "never": | ||
| console._color_system = None | ||
| # "auto" keeps the default TTY detection | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cd /tmp && git clone https://github.com/Textualize/rich.git 2>/dev/null && cd rich && git log --oneline -1 && python3 -c "import rich; print(rich.__version__)" 2>/dev/null || echo "Failed to get Rich directly"Repository: Shlomob/ocmonitor-share
Length of output: 164
🌐 Web query:
Rich console color_system values documentation
💡 Result:
In Rich, Console(color_system=...) accepts these documented values: None, "auto" (default), "standard", "256", "truecolor", "windows". Setting a higher capability than your terminal supports may make output unreadable. [1]
Docs: Rich “Console API → Color systems” section. [1]
(Reference API also notes the common values "standard", "256", "truecolor" and default "auto".) [2]
Sources: [1] (rich.readthedocs.io) [2] (rich.readthedocs.io)
Citations:
- 1: https://rich.readthedocs.io/en/latest/console.html?utm_source=openai
- 2: https://rich.readthedocs.io/en/stable/reference/console.html?utm_source=openai
🏁 Script executed:
# Check the actual code in ocmonitor/cli.py around lines 205-213
find . -name "cli.py" -path "*/ocmonitor/*" | head -1 | xargs -I {} sh -c 'wc -l {} && sed -n "195,220p" {}'Repository: Shlomob/ocmonitor-share
Length of output: 1044
🏁 Script executed:
# Also check what version of Rich might be used
find . -name "requirements*.txt" -o -name "setup.py" -o -name "pyproject.toml" | xargs grep -l "rich" 2>/dev/null | head -3Repository: Shlomob/ocmonitor-share
Length of output: 113
Use Rich's public constructor API for --color instead of mutating a private field with an invalid value.
"true" is not a valid Rich color_system value. The documented values are None, "auto", "standard", "256", "truecolor", and "windows". Mutating the private _color_system field after construction is fragile and does not use Rich's intended API. Use force_terminal=True to force color output (equivalent to --color always) and no_color=True to disable it (equivalent to --color never).
💡 Proposed fix
- console = Console(theme=theme_obj)
-
- # Apply --color setting
- if color == "always":
- console._color_system = "true"
- elif color == "never":
- console._color_system = None
- # "auto" keeps the default TTY detection
+ console_kwargs = {"theme": theme_obj}
+ if color == "always":
+ console_kwargs["force_terminal"] = True
+ elif color == "never":
+ console_kwargs["no_color"] = True
+
+ console = Console(**console_kwargs)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| console = Console(theme=theme_obj) | |
| # Apply --color setting | |
| if color == "always": | |
| console._color_system = "true" | |
| elif color == "never": | |
| console._color_system = None | |
| # "auto" keeps the default TTY detection | |
| console_kwargs = {"theme": theme_obj} | |
| if color == "always": | |
| console_kwargs["force_terminal"] = True | |
| elif color == "never": | |
| console_kwargs["no_color"] = True | |
| console = Console(**console_kwargs) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@ocmonitor/cli.py` around lines 205 - 213, The code mutates
Console._color_system after construction with an invalid value; instead
construct Console using its public flags: when color == "always" pass
force_terminal=True; when color == "never" pass no_color=True; otherwise leave
defaults (or pass force_terminal=False/no_color=False for "auto"). Update the
Console(...) call (the Console instantiation in ocmonitor/cli.py) to set these
parameters based on the color variable rather than mutating
console._color_system.
| @click.option( | ||
| "--days", "-d", type=int, default=None, help="Filter sessions from the last N days" | ||
| ) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -name "cli.py" | grep -E "ocmonitor|cli"Repository: Shlomob/ocmonitor-share
Length of output: 85
🏁 Script executed:
cat -n ocmonitor/cli.py | sed -n '280,295p'Repository: Shlomob/ocmonitor-share
Length of output: 630
🏁 Script executed:
cat -n ocmonitor/cli.py | sed -n '320,340p'Repository: Shlomob/ocmonitor-share
Length of output: 1034
🏁 Script executed:
head -30 ocmonitor/cli.py | cat -nRepository: Shlomob/ocmonitor-share
Length of output: 1114
🏁 Script executed:
rg "IntRange" ocmonitor/Repository: Shlomob/ocmonitor-share
Length of output: 49
🏁 Script executed:
rg "type=click\." ocmonitor/cli.py | head -10Repository: Shlomob/ocmonitor-share
Length of output: 714
🌐 Web query:
click.IntRange documentation parameter types
💡 Result:
In Click, click.IntRange is a parameter type (usable in @click.option(..., type=...) / @click.argument(..., type=...)) with this constructor signature:
click.IntRange(min=None, max=None, min_open=False, max_open=False, clamp=False)
Parameter meanings / types:
min:int | None(lower bound;None= unbounded)max:int | None(upper bound;None= unbounded)min_open:bool(exclude the lower boundary value)max_open:bool(exclude the upper boundary value)clamp:bool(ifTrue, out-of-range inputs are clamped to the nearest boundary instead of erroring) [1]
🏁 Script executed:
cat -n ocmonitor/cli.py | sed -n '329,337p'Repository: Shlomob/ocmonitor-share
Length of output: 544
Use click.IntRange(min=1) to reject invalid --days values at parse time.
Line 331 computes start_date = end_date - timedelta(days=days - 1). When days=0, this produces start_date > end_date. Negative values similarly invert the date range. Click's IntRange catches these at parse time instead of returning a confusing empty result.
Proposed fix
`@click.option`(
- "--days", "-d", type=int, default=None, help="Filter sessions from the last N days"
+ "--days",
+ "-d",
+ type=click.IntRange(min=1),
+ default=None,
+ help="Filter sessions from the last N days",
)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| @click.option( | |
| "--days", "-d", type=int, default=None, help="Filter sessions from the last N days" | |
| ) | |
| `@click.option`( | |
| "--days", | |
| "-d", | |
| type=click.IntRange(min=1), | |
| default=None, | |
| help="Filter sessions from the last N days", | |
| ) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@ocmonitor/cli.py` around lines 286 - 288, The --days option currently allows
0/negative values which later make start_date = end_date - timedelta(days=days -
1) produce an invalid/inverted range; change the click option type for "--days"
(the option that sets the variable days) from type=int to
type=click.IntRange(min=1) so invalid values are rejected at parse time, and
ensure click.IntRange is imported where the CLI is defined.
| # Load all sessions first (without limit) | ||
| sessions_list = analyzer.analyze_all_sessions(path) | ||
| console.print( | ||
| f"[status.info]Analyzing {len(sessions_list)} sessions...[/status.info]" | ||
| ) | ||
|
|
||
| if not sessions_list: | ||
| console.print("[status.error]No sessions found in the specified directory.[/status.error]") | ||
| ctx.exit(1) | ||
|
|
||
| result = report_generator.generate_sessions_summary_report( | ||
| path, limit, output_format, group_workflows=not no_group | ||
| ) | ||
| # Apply date filtering if --days is specified | ||
| if days is not None: | ||
| end_date = date.today() | ||
| start_date = end_date - timedelta(days=days - 1) | ||
| sessions_list = analyzer.filter_sessions_by_date( | ||
| sessions_list, start_date, end_date | ||
| ) | ||
| console.print( | ||
| f"[status.info]Filtering to {len(sessions_list)} sessions from the last {days} day(s)...[/status.info]" | ||
| ) | ||
|
|
||
| # Apply limit after date filtering | ||
| if limit is not None: | ||
| sessions_list = sessions_list[:limit] | ||
| console.print( | ||
| f"[status.info]Limited to {len(sessions_list)} most recent sessions[/status.info]" | ||
| ) |
There was a problem hiding this comment.
Keep loader-side limiting when --days isn't set.
Line 319 ignores SessionAnalyzer.analyze_all_sessions(..., limit) and always loads the full history before slicing. That turns --limit into an in-memory post-filter and will regress latency and memory usage on larger datasets.
💡 Proposed fix
- # Load all sessions first (without limit)
- sessions_list = analyzer.analyze_all_sessions(path)
+ # Preserve loader-side limiting unless date filtering is requested
+ load_limit = None if days is not None else limit
+ sessions_list = analyzer.analyze_all_sessions(path, load_limit)
console.print(
f"[status.info]Analyzing {len(sessions_list)} sessions...[/status.info]"
)
@@
- if limit is not None:
+ if days is not None and limit is not None:
sessions_list = sessions_list[:limit]
console.print(
f"[status.info]Limited to {len(sessions_list)} most recent sessions[/status.info]"
)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@ocmonitor/cli.py` around lines 318 - 344, The code always calls
SessionAnalyzer.analyze_all_sessions(path) and only slices in-memory, which
defeats loader-side limiting; change the call so when days is None you pass the
limit into analyzer.analyze_all_sessions(path, limit=limit) to let the loader
apply the limit, and keep the existing path where if days is specified you call
analyze_all_sessions(path) (no limit), then run
analyzer.filter_sessions_by_date(sessions_list, start_date, end_date) and only
after that apply sessions_list = sessions_list[:limit] if limit is set; update
references to analyze_all_sessions, filter_sessions_by_date, sessions_list,
limit, and days accordingly.
| if session_start_date: | ||
| # Include if: started in range OR ended in range OR spans the range | ||
| in_range = False | ||
|
|
||
| # Check if start date is in range | ||
| if TimeUtils.date_in_range(session_start_date, start_date, end_date): | ||
| in_range = True | ||
| # Check if end date is in range | ||
| elif session_end_date and TimeUtils.date_in_range( | ||
| session_end_date, start_date, end_date | ||
| ): | ||
| in_range = True | ||
| # Check if session spans the range (started before, ended after) | ||
| elif session_end_date and start_date and end_date: | ||
| if session_start_date < start_date and session_end_date > end_date: | ||
| in_range = True | ||
|
|
||
| if in_range: | ||
| filtered.append(session) |
There was a problem hiding this comment.
Don't drop sessions that only have end_time.
The new guard still requires session_start_date, so a session with a missing start timestamp but an end_time inside the requested window is silently excluded. That undercounts partially populated sessions even though session_end_date is already available.
💡 Proposed fix
- if session_start_date:
+ if session_start_date or session_end_date:
+ session_start_date = session_start_date or session_end_date
+ session_end_date = session_end_date or session_start_date
# Include if: started in range OR ended in range OR spans the range
in_range = False📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if session_start_date: | |
| # Include if: started in range OR ended in range OR spans the range | |
| in_range = False | |
| # Check if start date is in range | |
| if TimeUtils.date_in_range(session_start_date, start_date, end_date): | |
| in_range = True | |
| # Check if end date is in range | |
| elif session_end_date and TimeUtils.date_in_range( | |
| session_end_date, start_date, end_date | |
| ): | |
| in_range = True | |
| # Check if session spans the range (started before, ended after) | |
| elif session_end_date and start_date and end_date: | |
| if session_start_date < start_date and session_end_date > end_date: | |
| in_range = True | |
| if in_range: | |
| filtered.append(session) | |
| if session_start_date or session_end_date: | |
| session_start_date = session_start_date or session_end_date | |
| session_end_date = session_end_date or session_start_date | |
| # Include if: started in range OR ended in range OR spans the range | |
| in_range = False | |
| # Check if start date is in range | |
| if TimeUtils.date_in_range(session_start_date, start_date, end_date): | |
| in_range = True | |
| # Check if end date is in range | |
| elif session_end_date and TimeUtils.date_in_range( | |
| session_end_date, start_date, end_date | |
| ): | |
| in_range = True | |
| # Check if session spans the range (started before, ended after) | |
| elif session_end_date and start_date and end_date: | |
| if session_start_date < start_date and session_end_date > end_date: | |
| in_range = True | |
| if in_range: | |
| filtered.append(session) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@ocmonitor/services/session_analyzer.py` around lines 249 - 267, The current
filter block requires session_start_date and thus skips sessions that lack a
start timestamp but have session_end_date within the query window; update the
logic in the session filtering block (where session_start_date,
session_end_date, TimeUtils.date_in_range, start_date, end_date and
filtered.append(session) are used) to treat start and end independently: if
session_start_date exists check if it falls in range (or spans the range with
session_end_date), and separately if session_end_date exists check if it falls
in range; set in_range true if either check passes so sessions with only
end_time are included.
|
Hey @smolin, thanks for the contribution and the kind words These are both useful features — --color for watch -c is a great use case, and --days is something I've wanted to add. I have a few things I'd like addressed before merging: --color implementation — The current approach mutates console._color_system with "true", which is a private attribute and not a valid Rich value. Could you switch to using Rich's public constructor flags instead? force_terminal=True for always and no_color=True for never. --limit performance regression — The PR always loads all sessions before slicing, which bypasses the loader/SQLite-level limit we had before. When --days isn't set, limit should still be passed to analyze_all_sessions() so we don't do a full scan unnecessarily. --days validation — --days 0 or negative values produce an inverted date range. Swapping type=int for type=click.IntRange(min=1) would catch that at parse time. Private method calls — The sessions command now calls _display_workflow_sessions_table, _format_sessions_summary_json, etc. directly. I'd prefer keeping things going through the public generate_sessions_summary_report API — happy to discuss how to extend it to accept pre-filtered sessions if that would help. Let me know if you have any questions — happy to pair on any of these. |
2 participants
This does however remove the --no-color flag from the 'live' subcommand, but the new global --color works for this case. The motivation for doing this was the 'watch' usage in the examples.
Examples:
Hope you find this helpful. Great little utility, thanks for creating it!
Summary by CodeRabbit
New Features
--coloroption to control command-line output colors (always/never/auto).--daysoption to filter sessions by date range.Improvements