Skip to content

Conversation

@drorIvry
Copy link
Contributor

@drorIvry drorIvry commented Oct 20, 2025

Description

Motivation and Context

Type of Change

  • 🐛 Bug fix (non-breaking change which fixes an issue)
  • ✨ New feature (non-breaking change which adds functionality)
  • 💥 Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • 📝 Documentation update
  • 🎨 Code style/refactoring (no functional changes)
  • 🧪 Test updates
  • 🔧 Configuration/build changes

Changes Made

Screenshots/Examples (if applicable)

Checklist

  • I have read the CONTRIBUTING.md guide
  • My code follows the code style of this project (PEP 8, type hints, docstrings)
  • I have run uv run black . to format my code
  • I have run uv run flake8 . and fixed all issues
  • I have run uv run mypy --config-file .mypy.ini . and addressed type checking issues
  • I have run uv run bandit -c .bandit.yaml -r . for security checks
  • I have added tests that prove my fix is effective or that my feature works
  • I have run uv run pytest and all tests pass
  • I have manually tested my changes
  • I have updated the documentation accordingly
  • I have added/updated type hints for new/modified functions
  • My changes generate no new warnings
  • I have checked my code for security issues
  • Any dependent changes have been merged and published

Testing

Test Configuration:

  • Python version:
  • OS:
  • Other relevant details:

Test Steps:
1.
2.
3.

Additional Notes

Related Issues/PRs

  • Fixes #
  • Related to #
  • Depends on #

fixes #105

@drorIvry drorIvry requested a review from yuval-qf as a code owner October 20, 2025 11:35
@coderabbitai
Copy link
Contributor

coderabbitai bot commented Oct 20, 2025

Summary by CodeRabbit

Release Notes

  • Bug Fixes
    • Fixed "/" character insertion in evaluation and configuration input fields for proper text entry behavior.

Walkthrough

This PR updates the version from 0.1.12 to 0.1.13 and adds targeted "/" key handling in the TUI application to allow forward slash character insertion into specific text fields (AgentURL, JudgeModel, and ServerURL) without intercepting them.

Changes

Cohort / File(s) Summary
Version bump
VERSION
Updated version from 0.1.12 to 0.1.13
Input handling for "/" key
packages/tui/internal/tui/app.go
Added targeted "/" key insertion logic for NewEvaluationScreen (AgentURL and JudgeModel fields) and ConfigurationScreen (ServerURL field); "/" now inserts directly at cursor position in these contexts instead of being intercepted

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~8 minutes

Possibly related PRs

  • hotfix/paste agent url #102: Modifies input handling in the same file for AgentURL/JudgeModel fields, addressing similar cursor positioning and field editing concerns.

Suggested reviewers

  • yuval-qf

Poem

🐰 A slash finds its way,
Through fields where it should play,
In URLs and configs bright,
The cursor dances left and right,
Version bumped, all is well! ✨

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Description Check ⚠️ Warning The pull request description is largely incomplete and fails to follow the provided template. The author has not filled in any of the critical sections: the Description, Motivation and Context, Type of Change selection, Changes Made, Screenshots/Examples, Checklist items, Testing details, Additional Notes, or Related Issues/PRs are all empty beyond template placeholders. Instead, the description begins with a list of unrelated previous commit/PR references that appears to be extraneous content. While the template sections are technically present, they contain no meaningful information about the current changes, defeating the purpose of the description check. The author should provide a complete pull request description following the template structure. Specifically, they should: add a clear description of the changes, explain the motivation and context for the "/" key handling fix, select the appropriate Type of Change (likely "Bug fix"), list the specific changes made (version bump and slash insertion logic for AgentURL and ServerURL fields), confirm the checklist items, and provide any relevant issue links if this addresses existing problems. Removing the extraneous commit history references would also improve clarity.
✅ Passed checks (2 passed)
Check name Status Explanation
Title Check ✅ Passed The title "hotfix/slash agent url" directly refers to the main functional change in this pull request: the addition of targeted "/" key handling in the TUI application. The raw summary confirms this is the primary modification—specifically, "/" insertion logic was added for AgentURL in NewEvaluationScreen and ServerURL in ConfigurationScreen. While the title is narrowly focused on the agent URL aspect rather than encompassing all affected fields, and doesn't mention the accompanying version bump from 0.1.12 to 0.1.13, it clearly captures the primary functional change that the developer intended to highlight. The title is concise and specific enough that a teammate scanning history would understand this PR addresses slash key input handling for URL fields.
Docstring Coverage ✅ Passed No functions found in the changes. Docstring coverage check skipped.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch hotfix/slash-agent-url

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

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Nitpick comments (1)
packages/tui/internal/tui/app.go (1)

984-992: Consider using rune slicing for ConfigurationScreen ServerURL field.

The "/" handling for the ServerURL field uses direct string slicing (lines 988-990), which is consistent with how other character input is handled in ConfigurationScreen (see lines 1747-1750, 1801-1803). However, this approach could corrupt the string if users enter multi-byte UTF-8 characters.

While this is consistent with the existing codebase pattern for ConfigurationScreen, consider refactoring the entire ServerURL text handling to use rune slicing (similar to NewEvaluationScreen at lines 974-980) for more robust UTF-8 support. That said, since URLs are typically ASCII-only, this is low priority.

Note: This addresses the previous review comment asking about server URL handling.

Example refactor:

 			// Don't intercept "/" if we're editing text in ConfigurationScreen
 			if m.currentScreen == ConfigurationScreen && m.configState != nil &&
 				m.configState.IsEditing && m.configState.ActiveField == ConfigFieldServerURL {
 				// Handle "/" character directly in server URL field
-				m.configState.ServerURL = m.configState.ServerURL[:m.configState.CursorPos] +
-					"/" + m.configState.ServerURL[m.configState.CursorPos:]
+				runes := []rune(m.configState.ServerURL)
+				m.configState.ServerURL = string(runes[:m.configState.CursorPos]) +
+					"/" + string(runes[m.configState.CursorPos:])
 				m.configState.CursorPos++
 				return m, nil
 			}
📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 10c23d2 and 046110d.

📒 Files selected for processing (2)
  • VERSION (1 hunks)
  • packages/tui/internal/tui/app.go (1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
packages/tui/internal/tui/app.go (1)
packages/tui/internal/tui/configurations.go (1)
  • ConfigFieldServerURL (14-14)
🔇 Additional comments (2)
VERSION (1)

1-1: LGTM! Version bump looks appropriate for a hotfix.

packages/tui/internal/tui/app.go (1)

966-983: LGTM! Forward slash handling correctly implemented for NewEvaluationScreen fields.

The implementation properly uses rune slicing to handle multi-byte UTF-8 characters and correctly inserts "/" at the cursor position for both AgentURL and JudgeModel fields. This prevents the "/" key from being intercepted for command input when editing these fields.

@yuval-qf yuval-qf merged commit 360a230 into main Oct 20, 2025
9 checks passed
@yuval-qf yuval-qf deleted the hotfix/slash-agent-url branch October 20, 2025 11:43
@yuval-qf
Copy link
Collaborator

fixes #105

This was referenced Oct 21, 2025
@drorIvry drorIvry mentioned this pull request Nov 10, 2025
21 tasks
@coderabbitai coderabbitai bot mentioned this pull request Jan 6, 2026
21 tasks
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.

[Bug] - Rogue TUI - Cannot insert / in Agent URL and Rogue Server URL

2 participants