Skip to content

Clarify interactive terminal checks#28

Merged
julman99 merged 1 commit into
mainfrom
issue-12-interactive-tty-prompt
May 15, 2026
Merged

Clarify interactive terminal checks#28
julman99 merged 1 commit into
mainfrom
issue-12-interactive-tty-prompt

Conversation

@julman99
Copy link
Copy Markdown
Owner

@julman99 julman99 commented May 14, 2026

Summary

  • add helpers for output-terminal and ENTER-prompt interactivity checks
  • keep progress tied to stdout being a terminal
  • require both stdin and stdout to be terminals before prompting for ENTER

Testing

  • git diff --check
  • make clean
  • bash test/ci/all.test.sh

Summary by CodeRabbit

Release Notes

  • Bug Fixes
    • Improved terminal detection for progress display and interactive prompts. The program now more accurately determines when it can safely display progress indicators and prompt for user input, requiring both input and output to be connected to a terminal.

Review Change Stack

@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented May 14, 2026

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 31fd660b-0fcb-45d2-a461-2a7071eb017d

📥 Commits

Reviewing files that changed from the base of the PR and between 744c2ac and b54c01c.

📒 Files selected for processing (1)
  • src/main.c

📝 Walkthrough

Walkthrough

Two static TTY-detection helper functions are introduced: output_is_terminal() and can_prompt_for_enter(). The progress rendering now uses output_is_terminal() instead of a direct isatty() call, and the ENTER-prompt behavior now requires both stdin and stdout to be TTYs via can_prompt_for_enter() instead of checking only stdin.

Changes

TTY Detection Centralization

Layer / File(s) Summary
TTY helper functions and their usage
src/main.c
Introduces output_is_terminal() to check if stdout is a TTY and can_prompt_for_enter() to verify both stdin and stdout are TTYs. Updates show_progress assignment and the ENTER-prompt condition to use these helpers, centralizing TTY checks and refining the prompt logic to require both input and output streams to be interactive.

Estimated Code Review Effort

🎯 2 (Simple) | ⏱️ ~8 minutes

Poem

🐰 A rabbit refactors, helpers take their place,
TTY checks now have a central space,
Both streams must dance when prompts appear,
Clean abstractions make the path more clear!

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title 'Clarify interactive terminal checks' accurately summarizes the main changes: adding helper functions and refining terminal interaction checks.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch issue-12-interactive-tty-prompt

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@kluster-ai
Copy link
Copy Markdown

kluster-ai Bot commented May 14, 2026

kluster.ai PR Review Summary

This update improves the logic for handling interactive terminal behavior. I have introduced two helper functions, output_is_terminal and can_prompt_for_enter, to centralize and refine how the application detects its environment.

Previously, the check for prompting a user to press ENTER before freeing memory only considered whether stdin was a TTY. This could lead to a poor user experience if stdout was redirected while stdin remained interactive. The new logic ensures that the interactive prompt is only displayed if both stdin and stdout are connected to a terminal. Additionally, the progress display logic now uses the dedicated helper for consistency. These changes make the application more robust when used in pipelines or shell scripts where input or output might be redirected.

Warning: We haven't found any reviews performed on this branch from the IDE. Make sure to install the IDE and CLI integrations to catch issues earlier and accelerate your development workflow.

Powered by kluster.ai - Real-Time AI code review in your IDE

Unfold

@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps Bot commented May 14, 2026

Greptile Summary

This PR refactors two inline isatty checks in main.c into named helper functions and tightens the ENTER-prompt condition to require both stdin and stdout to be terminals.

  • output_is_terminal() wraps isatty(fileno(stdout)) and is used for the show_progress flag (no behavioral change there).
  • can_prompt_for_enter() adds a stdout-terminal check alongside the existing stdin check; when stdout is redirected, the program now falls through to the infinite-sleep loop instead of silently waiting for an ENTER the user cannot see.

Confidence Score: 5/5

Safe to merge — changes are a pure refactor of two inline isatty calls into well-named helpers with one intentional and documented behavioral tightening.

The change is narrow and self-contained: two new static helpers with no side effects, one call-site substitution that is functionally identical, and one call-site that intentionally tightens the condition (requiring stdout to also be a terminal before the ENTER prompt). The tightened condition changes the fallback path when stdout is redirected but that is explicitly the stated goal of the PR and a correctness improvement.

No files require special attention.

Important Files Changed

Filename Overview
src/main.c Two small static helpers introduced; show_progress and ENTER-prompt condition updated to use them. No correctness issues found.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[timeout < 0?] -->|yes| B{can_prompt_for_enter?\nisatty stdin AND isatty stdout}
    A -->|no| E[sleep timeout seconds, exit]
    B -->|yes| C[Print 'press ENTER'\nwait getchar]
    B -->|no| D[Print 'kill this process'\nwhile true sleep 1]
    C --> F[digest / free memory]
    D --> F
    E --> F
Loading

Reviews (1): Last reviewed commit: "Clarify interactive terminal checks" | Re-trigger Greptile

@julman99 julman99 changed the base branch from issue-12-lock-memory to main May 14, 2026 21:05
@julman99 julman99 force-pushed the issue-12-interactive-tty-prompt branch from 6357074 to b54c01c Compare May 14, 2026 21:12
@julman99 julman99 merged commit 2829ffb into main May 15, 2026
100 of 101 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant