Repository navigation
🌂 feat: Require Namespace Cleanup for Long Type Imports - #16576
lia-by-librechat[bot] wants to merge 9 commits into
Conversation
|
Review head 3d2eaf2: adds an opt-in compact-type-import mode to the scoped sorter, symbol-aware reference updates, conservative exclusions, and focused tests/typechecking in Static Checks. Runtime imports and default cleanup behavior are unchanged. |
|
Review head c9a9312: preserves synthetic default aliases, normalizes Windows compiler-host paths, and adds type-resolution and cross-platform filename regressions. All 16 focused tests, tooling no-emit compilation, and touched-file lint passed locally. The prior head's two P2 review findings are fixed here. |
|
Final verification for head c9a9312:
|
|
Review head 007f889: safe type-import compaction now runs during normal cleanup and normal --check. The existing pre-commit sorter automatically fixes eligible staged files, and Static Checks rejects eligible imports in changed source files. Runtime imports and conservative safety exclusions remain unchanged. All 16 focused tests, tooling typecheck, touched-file lint/formatting, and available local Static Checks passed. Title rarity rechecked: 🌂 has 0 prior uses (0 subject-leading) in 5,750 indexed LibreChat commits through 2026-09-30 22:35:22 UTC; sentinel passed. |
|
Review head c32c248: normal cleanup and check mode require safe type-import compaction. The current-dev workflow conflict is resolved while retaining its design-rule build/suppression gates and the new import-tooling tests. All 16 focused tests, tooling no-emit compilation, touched-file lint/formatting, and resolved-workflow YAML validation passed. Runtime imports and safety exclusions remain unchanged; no bulk application migration is included. |
|
Required-cleanup handoff for head c32c248:
|
|
@codex review the latest head, final review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c32c248102
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
Review head f0b8b06: fixes both Codex P2 findings by measuring full import attributes and selecting import-tooling tests/typecheck/formatting on either root dependency manifest. All 18 focused tests and tooling no-emit compilation passed; focused coverage includes attributed import boundaries and real hook/check enforcement. The runtime follow-up #16622 remains unchanged. Prior-head CI and review do not cover this revision. |
|
Review head 33b71c7: both Codex P2 fixes are retained, with lint-clean workflow regression guards. All 18 focused tests, tooling tsc --noEmit, touched-file lint/formatting and manifest/workflow validation passed. Findings C1 (complete attribute width) and C2 (dependency-only tooling CI) fixed in f0b8b06; no rejections. Fresh independent review and CI cover this final head. Runtime follow-up #16622 is untouched. |
|
@codex review the latest head, final review |
|
Codex-fix status at head 33b71c7:
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 33b71c7ff0
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
Review head 0d80a96: fixes C3 (P2, formatter-only trigger) and C4 (P2, missing local tooling gate) with one shared local/CI import-tools check. All 21 focused tests, tooling no-emit compilation, formatting, new tooling lint, and workflow/manifest parity passed. Static runner lint adds no diagnostics over its 14 inherited findings. Failure-injection tests exercise tests/types/formatting; config validation rejects missing or mistyped printWidth. Changed-file CI now uses the tested merge base parent, excluding unrelated newer dev sources that caused the previous import-check failure. Prior C1/C2 fixes retained. No rejected findings. #16622 remains untouched. Fresh review and CI must cover this exact head. |
|
Review head 8bbaa38: C3/C4 are fixed in 0d80a96 and retained after merging current dev. Local and CI now execute one import-tools gate; formatter-only changes select it, and invalid printWidth fails explicitly. All 21 import regressions, tooling tsc --noEmit, formatter/manifest/path parity, new tooling lint and both upstream i18n tests passed on the conflict-resolved tree. Runner lint has no new diagnostics over its 14 inherited findings. The upstream CRLF parser and i18n gate are preserved. Changed-file CI excludes unrelated newer dev files using the tested merge-parent baseline. No documentation or application source changes beyond inherited dev updates. Earlier-head review and CI do not cover this head. #16622 is untouched. |
|
Review head 2bf3617: fixes R3 (P2, CRLF fixture rejection) by preserving endOfLine:auto in its isolated formatter config. The real shared gate now runs against both LF and CRLF runner copies before test/type/formatting failure injection. Locked Prettier reproduction confirmed the former fixture rejected CRLF and the corrected policy accepts it. All 21 focused tests, tooling no-emit compilation and formatting passed through import-tools; touched test lint passed. Earlier C1-C4 fixes retained, no rejected findings. Exact-head CI and a fresh independent review are required. Runtime follow-up #16622 is unchanged. |
|
Final review of head 2bf3617: complete, no findings. Independent verification used the successful exact-head tooling CI step; its lane lacked local TypeScript/Prettier. Local: 21 focused tests, tooling typecheck, LF/CRLF gate execution, formatting and touched test lint passed. |
|
@codex review the latest head, final review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2bf3617e98
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const flat = `import type { ${bindings.elements.map((binding) => binding.getText(source)).join(', ')} } from ${statement.moduleSpecifier.getText(source)}${suffix};`; | ||
| if (flat.length <= printWidth) continue; |
There was a problem hiding this comment.
Normalize spacing before deciding to compact
When an aliased specifier contains extra internal whitespace, binding.getText(source) preserves that whitespace in flat, so the compactor can classify a formatter-normalized short import as long. Fresh evidence at this head is import type { Alpha as X, Beta } from './models': at width 60 this is converted to a namespace even though Prettier normalizes it to a 49-character named import; because the documented hook runs the sorter before Prettier, the unwanted conversion is permanent. Measure the normalized declaration so short type imports remain unchanged.
AGENTS.md reference: AGENTS.md:L184-L190
Useful? React with 👍 / 👎.
| git diff -z --name-only --diff-filter=ACMRTUXB HEAD^1 HEAD | | ||
| grep -zE '^(api|client|packages)/.*\.(js|jsx|ts|tsx)$' || true |
There was a problem hiding this comment.
Include module-suffixed TypeScript files in the cleanup gate
When a PR adds or edits a long type import in a supported source such as packages/api/src/worker.mts, this changed-file pipeline drops it because the following filter accepts only js|jsx|ts|tsx. The sorter itself explicitly supports .mts and .cts, but the local runner uses the same narrower source-file classification and lint-staged also omits these suffixes, so neither pre-commit nor Static Checks enforces the new compaction rule for those files. Include the module suffixes in the import-check selection.
AGENTS.md reference: AGENTS.md:L187-L191
Useful? React with 👍 / 👎.
| import_tools: | ||
| - '.prettierrc' | ||
| - 'scripts/static-checks.mts' | ||
| - 'scripts/sort-imports.mts' | ||
| - 'scripts/imports/**' |
There was a problem hiding this comment.
Retest changes to the compaction hook
When a PR changes only .husky/lint-staged.config.js—for example, removing or reordering the node scripts/sort-imports.mts task—neither the workflow trigger nor this import_tools filter selects the change. The new regression suite directly loads that file to verify that the pre-commit path performs compaction, so skipping the suite for the hook's own changes lets the promised staged-file enforcement break unnoticed; include the lint-staged config in both path lists.
AGENTS.md reference: AGENTS.md:L187-L188
Useful? React with 👍 / 👎.
Summary
Normal scoped import cleanup compacts safely convertible long type-only imports into namespaces and updates references using TypeScript symbols. The staged-file hook applies the same cleanup; changed-file CI rejects eligible imports left unconverted. Runtime imports remain unchanged. Runtime support is separate in #16622.
Run Prettier on affected files afterward, as with the existing sorter. Check mode does not write files. No application source is bulk-migrated.
How it works
The threshold is
.prettierrc'sprintWidth, measured against the complete normalized import declaration, includingwithorassertattributes. The converter binds only the current file without resolving modules or loading a workspace project. Aliases and shadowed locals are handled by symbol identity; namespace names avoid collisions.Default aliases, re-exports, unsupported uses, commented import clauses, ambiguous declarations, and malformed files remain unchanged. Existing
sort-imports-ignoremarkers apply. Compiler-host paths are normalized for Windows.TypeScript is an explicit root development dependency at its already-locked version. The local Static Checks runner and CI use the same fast
import-toolsgate for focused tests, no-emit typechecking, and formatting. Both select tooling, runner, workflow,.prettierrc, and either dependency manifest. Formatter-only changes start CI. Cleanup rejects missing or invalidprintWidthinstead of silently changing the rule. Changed-file CI compares the synthetic PR merge with its tested base parent, excluding newer upstream files unrelated to this PR.Type of change
Testing
Node.js 24.16.0, TypeScript 5.9.3, Prettier 3.6.2.
npm run static-checks -- --against origin/dev --only import-tools --verbose: exercises the shared gate. Its 21 focused tests cover formatter/dependency selection, real runner LF/CRLF execution and failure propagation for tests/types/formatting, config validation, synthetic-merge selection, attributed import boundaries, hook/check enforcement, aliases, shadowing, Windows paths and conservative exclusions.npx --no-install tsc --noEmit --project scripts/imports/tsconfig.json: passed.npm run static-checks -- package.json --only json: passed. The scoped sorter ignores tooling files outside application source roots; its application-file behavior is exercised by CLI regressions.Not run locally: full application tests, workspace builds/typechecks, Lighthouse, circular-dependency detection, and upstream design-rule build/suppression gates. The isolated installation contains tooling dependencies only; applicable CI lanes cover those checks.
Screenshots / recordings
No user-facing change.
Risk / compatibility
Required cleanup applies to processed source files. Runtime imports and runtime evaluation are unchanged. Unsupported patterns are skipped rather than rewritten unsafely. Changed-file CI does not reject untouched source; whole-tree checks intentionally report pre-existing eligible imports.
Checklist
Verification status
Pushed and independently reviewed head:
2bf3617e985b25f05788e8479807368811d76829. Independent review completed with no findings. It confirmed the successful exact-head tooling CI step; its isolated lane lacked compiler/formatter dependencies, so those checks were not independently rerun locally.Local shared-gate verification passed all 21 tests, tooling no-emit compilation and formatting, including actual LF and CRLF runner execution. Touched test lint passed. C1/C2 (P2, complete import width and dependency triggers) are fixed in
f0b8b06aa20ae41d3349be4ce03d4ab387a3abb6; C3/C4 (P2, formatter trigger and local/CI gate parity) in0d80a96e909a579ef3ec988c396f1006d91c6e1b; R3 (P2, CRLF fixture) in this final head. Earlier synthetic-default and Windows path safeguards remain intact. All Codex threads are resolved; no findings are rejected or open.As of 2026-10-05 02:12 UTC, exact-head Static Checks, workspace typechecks, builds, backend tests, circular-dependency detection, Lighthouse, runtime smoke and Redis E2E passed. Memory E2E and Ubuntu frontend test shards remain pending, with no failures reported. Full application checks were not run locally. The runtime follow-up #16622 remains unchanged.