Repository navigation
Bring main back under the line limits (main lint is red) - #31729
Conversation
…ne limit Move the client-hosted guest's six event-listener add/remove calls into bindBrowserClientPageGuestEvents beside the guest load-failure handler, so ClientHostedBrowserPagePane.tsx drops from 404 to 395 counted lines. No behavior change.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
📝 Walkthrough
Priority: ⬇️ Low Merge Risk: ⚪ Minimal · up to
|
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Description check |  |
The description gives detailed problem, change, rationale, testing, and review context, but it omits most required template sections. It also states that there is no linked issue, does not provide the… | Use the repository template headings. Add the required linked issue or confirm the maintainer exception, state N/A with a reason under Visual Proof, complete Testing and Checklist items, provide AI Disclosure details when applicable, and … |
-
Check name Status Explanation Title check The title clearly describes the main change: restoring the main branch below the lint line limits. Linked Issues check Issue #31727is closed and supplies historical context only. No active directly linked issue provides coding requirements for this pull request.Out of Scope Changes check The current pull request description scopes the changes to splitting oversized modules while preserving behavior. The browser event helper, its contract test, and the native-chat answer-module extract… Docstring Coverage Docstring coverage is 80.00% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 10 functions across 5 files. Full details: Out of Scope Changes check
-
Explanation
The current pull request description scopes the changes to splitting oversized modules while preserving behavior. The browser event helper, its contract test, and the native-chat answer-module extraction all support that lint-oriented scope. No unrelated change is established.
-
Full details: Description check
-
Explanation
The description gives detailed problem, change, rationale, testing, and review context, but it omits most required template sections. It also states that there is no linked issue, does not provide the required Visual Proof
N/Aentry, and does not complete the testing, AI disclosure, notes, or checklist sections.Resolution
Use the repository template headings. Add the required linked issue or confirm the maintainer exception, state
N/Awith a reason under Visual Proof, complete Testing and Checklist items, provide AI Disclosure details when applicable, and complete Notes and Agent skill upstream boundary sections.
✨ 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.
Comment @coderabbitai help to get the list of available commands.
…-browser-max-lines
…imit Move answer encoding (pasted answer text and the Claude and Codex selector keystrokes) into native-chat-ask-answer.ts and re-export it, so importers are unchanged.
brennanb2025
changed the title
|
Main's lint failed after several nearby merges together pushed the browser pane and question module past their line limits. This PR restores those limits by moving the existing event bindings and answer encoder into focused modules. Users see the same browser navigation, recovery and question-answer behavior. The full review covered correctness, architecture, cleanup, performance, desktop/mobile consumers and the final reference comparison. No PR-caused regressions or required fixes were found. The answer encoder matches the original 7,527 bytes exactly; existing exports remain available. Verified on
Deferred: an existing minor desktop/phone difference in multi-select pasted-answer order (choose B then A: desktop sends A, B; phone sends B, A). This PR preserves both behaviors. For Brennan's review: this is a behavior-preserving refactor, so the agreed QA is tests; no running-app comparison or screenshots were taken. The two useful review points are the guest binding and cleanup and the compatibility exports. |
Problem
Main's lint step fails the
max-linesrule, so CI is red for every PR that merges current main. Several PRs merged close together on 10-21, each passed on its own, and together they pushed files over their line limits (the rule skips blank and comment lines):.tsxfiles over the 400-line limit:assemble-chrome/BrowserAddressBar.tsx(403 lines): already fixed on main by fix(browser): bring the address bar back under the 400-line limit #31727.browser-pane/ClientHostedBrowserPagePane.tsx(404 lines): fixed here.src/shared/native-chat-ask.tsto 310 lines against the 300-line limit for.tsfiles: fixed here.Evidence: pr.yml runs 39696231509 (browser pane) and 39698353017 (
src/shared/native-chat-ask.ts:419:2: File has too many lines (310)).The max-lines ratchet bans suppression comments and growing the exceptions list, so each file is split instead.
User-facing change
None. This only moves code.
What moved where
browser-pane/ClientHostedBrowserPagePane.tsx(404 → 395 counted lines)The client-hosted page pane attached and detached the guest webview's six event listeners (
did-start-navigation,did-stop-loading,did-navigate,did-navigate-in-page,page-title-updated,did-fail-load) inline in its attach effect. Those calls now live inbindBrowserClientPageGuestEventsinbrowser-pane/browser-client-page-guest-metadata.ts(now 90 raw lines), next to the guest's load-failure handler factory. It binds the same handlers to the same events and returns the matching unbind, which the pane's existing cleanup calls first, as before. The listeners are bound a few statements earlier, but still in the same synchronous block of the effect, before anything can fire; webview events arrive asynchronously, so the order can't be observed. One unit test pins the helper's contract (it binds the six events, and unbind removes exactly those handlers).src/shared/native-chat-ask.ts(310 → 208 counted lines)The answer-encoding half of the module moved verbatim into a new sibling,
src/shared/native-chat-ask-answer.ts(111 counted lines): theAskAnswerSelectionandAskAnswerKeyGrouptypes,formatAskAnswer(the pasted answer text),buildAskAnswerKeys(Claude's selector keystrokes),buildCodexAskAnswerKeys(Codex's overlay keystrokes),hasAskAnswer, and their private helpers and key constants. The question parsing and pending-question scan stay innative-chat-ask.ts, which re-exports the moved names, so every desktop and mobile importer is unchanged.Review path: full review loop
This PR goes through the full review loop, not the light path. A light-path reviewer found that the browser pane is a risky area with past incidents (#28913, #25827), so the earlier light-path claim for this PR no longer holds.
Verification
ClientHostedBrowserPagePane*andrestored-client-hosted-recovery-windowtest files pass (83 tests), and the newbrowser-client-page-guest-metadata.test.tspasses.native-chat-ask.test.ts,native-chat-ask-fifo.test.ts,agent-status-types.test.ts,codex-structured-async-question.test.ts,native-chat-interactive-prompt.test.tsandNativeChatQuestionCard.test.tsxpass (145 tests). Mobile importers are covered by CI's Mobile Checks.oxlintwith the repo config is clean on both changed question-module files; the pre-commit hook (oxlint and oxfmt on staged files) passes.Splitting an oversized module into a focused sibling and re-exporting from the original is the common pattern in this repo's
src/sharedmodules.Ticket: none.
Author: Brennan (X: @BrennanKB5)