Skip to content

fix(lint): main lint is red — split native-chat-ask.ts under the max-lines limit - #31734

Closed
brennanb2025 wants to merge 1 commit into
mainfrom
brennanb2025/main-native-chat-ask-max-lines
Closed

brennanb2025 wants to merge 1 commit into
mainfrom
brennanb2025/main-native-chat-ask-max-lines

Conversation

@brennanb2025

@brennanb2025 brennanb2025 commented Oct 22, 2026 •

Copy link
Copy Markdown
Contributor
Files Added Deleted Net
Test 1 $\color{#1a7f37}{\Huge{\mathbf{+}}}$​2 $\color{#cf222e}{\Huge{\mathbf{−}}}$​1 $\color{#1a7f37}{\Huge{\mathbf{+}}}$​1
Prod 4 $\color{#1a7f37}{\Huge{\mathbf{+}}}$​147 $\color{#cf222e}{\Huge{\mathbf{−}}}$​141 $\color{#1a7f37}{\Huge{\mathbf{+}}}$​6

ELI5

Main's lint is red: one shared file about chat questions grew past the 300-code-line limit. This moves one self-contained part of it (the code that turns a question answer into keystrokes for the agent's terminal) into its own file. Nothing behaves differently.

What Changed

User-facing change: none. Developer-facing change: main's lint goes green again for this file.

Why

The keystroke builders are the one part of the file with a single job of their own (driving each agent's terminal question selector) and only one dependency back (answerLabels), so they split out cleanly with a pure move. No max-lines disable.

Plan (light path): move the keystroke builders into a sibling module, update imports, run the covering tests, let CI be the lint and typecheck gate.

  • Production lines: pure move, about 90 lines moved, 5 production files touched (≤ 150 lines, ≤ 5 files).
  • No saved data, wire or RPC payloads, lifecycle, processes, build or packaging, or cross-version behavior: code is moved verbatim.
  • Not a repeat fix in one mechanism: it is a file-size fix.

ClientHostedBrowserPagePane.tsx also fails max-lines on main; that is fixed separately in #31729 and not touched here.

Linked Issue

N/A (main's lint is red; no issue).

Visual Proof

N/A: code move only, no UI or behavior change.

Testing

  • src/shared/native-chat-ask.test.ts, src/shared/native-chat-ask-fifo.test.ts, src/renderer/src/components/native-chat/native-chat-interactive-prompt.test.ts (covers both keystroke builders): 53 passed locally.

  • Mobile tests touching the moved imports run in CI.

  • I manually tested these changes locally

  • Automated tests added/updated, or explained why not below (existing tests cover the moved code; no new behavior)

AI Disclosure

Review

Agent skill upstream boundary

  • Not applicable, or this change copies or mechanically translates no upstream skill-installer source, tests, fixtures, registry entries, path tables, comments, or documentation.

Notes

Pure code move; no security, platform, SSH, mobile behavior, compatibility or performance impact.

Checklist

  • This PR is small and focused
  • I explained what changed and why (ELI5, the user-facing before/after, the mechanism, and why over the alternatives)
  • Before/after screenshots or videos attached for UI changes, or N/A with reason
  • Self-reviewed for correctness, security, and performance
  • Cross-platform, SSH/remote, and path/shortcut impact considered (or N/A)
  • pnpm lint, pnpm typecheck, pnpm test, and pnpm build pass (or CI will cover; local preferred)

…-ask.ts

Main's lint fails max-lines on src/shared/native-chat-ask.ts (310 code lines
against 300). Move the PTY keystroke builders for Claude's and Codex's question
selectors into native-chat-ask-keystrokes.ts unchanged and update importers.
@brennanb2025
brennanb2025 marked this pull request as ready for review October 22, 2026 00:17
@coderabbitai

coderabbitai Bot commented Oct 22, 2026 •

Copy link
Copy Markdown
Contributor

Review in Change Stack →

📝 Walkthrough

The Claude and Codex keystroke builders moved from native-chat-ask to native-chat-ask-keystrokes. The new module exports AskAnswerKeyGroup and both builders. answerLabels is now exported from native-chat-ask. Mobile and renderer imports were updated; the test import was split into separate type and value imports.

Priority: ⬇️ Low

Merge Risk: 🔵 Low · up to 04b5b

The keystroke move leaves one test unable to import its builder. Update that import before relying on the test; the identified risk is localized.

Pre-merge checks: Passed 5
Check name Status Explanation
Title check Passed The title clearly and concisely describes the main change: moving code from native-chat-ask.ts to resolve the max-lines lint failure.
Description check Passed The description is detailed and covers the change, rationale, testing, visual impact, scope, and checklist. It explicitly documents that no linked issue exists and that full local validation commands …
Docstring Coverage Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 6 functions across 5 files.
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.
Full details: Description check

Explanation

The description is detailed and covers the change, rationale, testing, visual impact, scope, and checklist. It explicitly documents that no linked issue exists and that full local validation commands were not run, so CI coverage is expected.

✨ Finishing Touches
📝 Generate docstrings
  • Commit to this branch
  • Create a new PR
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

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.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Actionable comments posted: 1


ℹ️ Review info
⚙️ Run configuration
  • Configuration used: Repository UI
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: 12e728eb-79e6-4407-8e81-0bc9d909055a
📥 Commits

Reviewing files that changed from the base of the PR and between b0fcdb4 and 04b5b56.

📒 Files selected for processing: 5
  • mobile/src/session/mobile-prompt-controller.test.ts
  • mobile/src/session/use-mobile-native-chat-answer-send.ts
  • src/renderer/src/components/native-chat/native-chat-interactive-prompt.ts
  • src/shared/native-chat-ask-keystrokes.ts
  • src/shared/native-chat-ask.ts

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 3 remain after this review.


/** The picked labels + trimmed free text for one question, in option order. */
function answerLabels(question: AskQuestion, sel: AskAnswerSelection | undefined): string[] {
export function answerLabels(question: AskQuestion, sel: AskAnswerSelection | undefined): string[] {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

🔎 Supported by static analysis

🏁 Script executed:

#!/bin/bash
# Find imports of the moved symbols that still point at native-chat-ask (not the keystrokes module).
rg -nU --type=ts -C2 "(buildAskAnswerKeys|buildCodexAskAnswerKeys|AskAnswerKeyGroup)" \
  -g '!src/shared/native-chat-ask-keystrokes.ts' | rg -n "native-chat-ask['\"]|native-chat-ask-keystrokes"

# List every importer of the old module for manual review.
rg -n --type=ts "from '.*native-chat-ask'" -g '!node_modules'

Repository: stablyai/orca

Length of output: 4662


🏁 Script executed:

#!/bin/bash
set -eu
printf '%s\n' '--- stale importer ---'
nl -ba tests/e2e/native-chat-ssh-delivery.unit.test.ts | sed -n '1,35p'
printf '%s\n' '--- old module moved-symbol references/exports ---'
rg -n -C2 --type=ts 'buildAskAnswerKeys|buildCodexAskAnswerKeys|AskAnswerKeyGroup|^export ' src/shared/native-chat-ask.ts || true
printf '%s\n' '--- new module ---'
nl -ba src/shared/native-chat-ask-keystrokes.ts | sed -n '1,140p'

Repository: stablyai/orca

Length of output: 11485


Update the remaining stale import.

tests/e2e/native-chat-ssh-delivery.unit.test.ts imports buildAskAnswerKeys from the old module. Import it from native-chat-ask-keystrokes instead.

Suggested fix
-import { buildAskAnswerKeys } from '../../src/shared/native-chat-ask'
+import { buildAskAnswerKeys } from '../../src/shared/native-chat-ask-keystrokes'

@brennanb2025

Copy link
Copy Markdown
Contributor Author

Closing as a duplicate: #31729 already moves the same code out of native-chat-ask.ts (and fixes the browser pane), so this PR's whole change is held there.

@brennanb2025
brennanb2025 deleted the brennanb2025/main-native-chat-ask-max-lines branch October 22, 2026 00:35
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