Skip to content

refactor(shared): bring constants.ts back under the max-lines limit - #23923

Merged
brennanb2025 merged 4 commits into
mainfrom
brennanb2025/constants-split
Sep 29, 2026
Merged

brennanb2025 merged 4 commits into
mainfrom
brennanb2025/constants-split

Conversation

@brennanb2025

@brennanb2025 brennanb2025 commented Sep 29, 2026 •

Copy link
Copy Markdown
Contributor
Files Added Deleted Net
Test 15 $\color{#1a7f37}{\Huge{\mathbf{+}}}$​27 $\color{#cf222e}{\Huge{\mathbf{−}}}$​24 $\color{#1a7f37}{\Huge{\mathbf{+}}}$​3
Prod 16 $\color{#1a7f37}{\Huge{\mathbf{+}}}$​103 $\color{#cf222e}{\Huge{\mathbf{−}}}$​86 $\color{#1a7f37}{\Huge{\mathbf{+}}}$​17

ELI5

One shared settings file grew past the size limit our lint enforces, and a translation catalog fell behind the app's own text; both turned every pull request's lint check red. This moves three self-contained groups of defaults into their own small files so it fits again. Nothing the app does changes.

What Changed

  • src/shared/constants.ts was 348 lines after feat(explorer): scope file trees to sparse checkout directories #18750, over the 300-line max-lines limit, so the full-file lint in "static analysis" failed on every PR that merged main. It is now 294 lines.
  • Three whole groups moved out, each into a file named for what it holds:
    • src/shared/onboarding-defaults.ts: ONBOARDING_FINAL_STEP, ONBOARDING_FLOW_VERSION, getDefaultOnboardingState.
    • src/shared/notification-settings-defaults.ts: getDefaultNotificationSettings.
    • src/shared/terminal-platform-defaults.ts: the per-platform terminal font and paste defaults (defaultTerminalFontFamily, getDefaultPrimarySelectionMiddleClickPaste, getDefaultTerminalRightClickToPaste).
  • The code moved verbatim; the only edit is exporting defaultTerminalFontFamily, which getDefaultSettings still calls. Every import site now names the new file directly; nothing is re-exported through constants.ts.
  • The new files import only types, so constants.ts can import them without a cycle.
  • src/renderer/src/i18n/en-runtime-required.json is regenerated with pnpm run sync:localization-runtime-catalog. On main it no longer covered keys main already ships (sparsePreset.findPath from feat(explorer): scope file trees to sparse checkout directories #18750, and browser-pane.markup.*), which turned "Verify localization catalogs" red on every PR. This change adds only those keys and reorders a few existing ones.
  • The lint exemption for the persisted shapedSidebar checklist field (config/oxlint-anti-slop.json) moves from constants.ts to onboarding-defaults.ts, which now holds that field.

Why

These three groups each have their own domain and few users (about 30 import lines in total), so moving them is a small, mechanical change. Moving the larger persisted-state default builders instead would have rewritten imports in about 160 files, and moving only the UI-state defaults would have created an import cycle with constants.ts. Raising the limit or disabling the rule for this file is not allowed in this repo.

Linked Issue

None; this fixes a red check on main.

Visual Proof

N/A: no behaviour change; the moved code is byte-identical apart from one added export.

Testing

  • tc:node, tc:web, tc:cli and the mobile tsc --noEmit pass.

  • Full-file oxlint --deny-warnings (including max-lines) and the react-doctor config pass on every touched file; oxfmt is clean.

  • Vitest over the changed test files, src/main/persistence and src/shared: 12,064 passed. The only failures were five SQLite backup/recovery tests timing out at 30 s under machine load. They time out the same way on current main without this change.

  • I manually tested these changes locally

  • Automated tests added/updated, or explained why not below

No new tests: this only moves code, and the existing tests import the moved functions from their new files.

Review

Agent skill upstream boundary

  • Not applicable, or this change follows docs/reference/agent-skill-sharing-upstream-boundary.md and copies or mechanically translates no upstream skill-installer source, tests, fixtures, registry entries, path tables, comments, or documentation.

Notes

No runtime, platform, SSH, mobile or wire impact: only module boundaries moved.

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)

@coderabbitai

coderabbitai Bot commented Sep 29, 2026 •

Copy link
Copy Markdown
Contributor

Review in Change Stack →

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

📝 Walkthrough

Walkthrough

Default notification, onboarding, and terminal platform implementations move from shared/constants into dedicated shared modules. Application code, tests, and E2E helpers now import those defaults from the new modules. The English runtime translations also add entries and move or reorder existing entries. The summaries report no changes to runtime logic or existing test cases.

Priority: ⬇️ Low

Merge Risk: 🔵 Low · up to 48b09

The sparse folder picker may show an untranslated label; rename the catalog key before merging.

Architecture Summary

Architecture risk: 🔵 Low · up to 48b09

The change affects 3 systems.

Changed systems: src, tests, config

Architecture concerns
No architecture-level concerns identified.

Review details

Systems and components

  • observed — src (service) was modified; 27 changed files map to changed impact.
  • observed — tests (service) was modified; 3 changed files map to changed impact.
  • observed — config (service) was modified; 1 changed file maps to changed impact.

Before / after behavior

  • observed — Modified behavior in config/oxlint-anti-slop.json: The file-scoped no-shape-in-symbol-names exemption replaces src/shared/constants.ts with src/shared/onboarding-defaults.ts; src/shared/onboarding-state-types.ts remains included.
  • observed — Modified behavior in src/main/persistence-settings-ui-defaults.test.ts: The onboarding constants are now imported from shared/onboarding-defaults; the old combined import from shared/constants is removed.
  • observed — Modified behavior in src/main/persistence/applying-settings/onboarding-normalization.ts: Imports getDefaultNotificationSettings from notification-settings-defaults and onboarding defaults and constants from onboarding-defaults, replacing imports of all four from shared/constants.
  • observed — Modified behavior in src/main/persistence/loading-store/profile-preferences.ts: The getDefaultOnboardingState import now comes from shared/onboarding-defaults instead of shared/constants.
🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 33.33% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 6 functions across 29 files. (1 skipped: … Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
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.
Title check ✅ Passed The title clearly identifies the primary change: refactoring shared constants to bring constants.ts below the repository's max-lines limit.
Description check ✅ Passed The description is detailed and follows the template. It explains the change, rationale, testing, scope, and compatibility impact. It notes that no linked issue exists and records known test timeouts.
Full details: Docstring Coverage

Explanation

Docstring coverage is 33.33% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 6 functions across 29 files. (1 skipped: 1 unsupported.)

  • Fix all pre-merge checks with AI

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.

✨ Finishing Touches 💡 1
🛠️ Fix failing CI checks 💡
  • Commit to this branch
  • Create a new PR

@greptile-apps

greptile-apps Bot commented Sep 29, 2026 •

Copy link
Copy Markdown

RetriggerConfidence Score: 5/5

[Medium risk] Splits shared constants into three focused modules.

The PR appears safe to merge; no outstanding finding or new actionable issue was identified.

Summary

The PR moves onboarding, notification, and terminal-platform defaults out of constants.ts, updates their import sites, relocates the corresponding lint exemption, and regenerates the runtime English catalog.

  • The moved default values and behavior remain unchanged.
  • No new actionable issue was identified.
Diagram
%%{init: {'theme': 'neutral'}}%%
flowchart LR
  C["constants.ts"] --> O["onboarding-defaults.ts"]
  C --> N["notification-settings-defaults.ts"]
  C --> T["terminal-platform-defaults.ts"]
  Callers["Other callers"] --> O
  Callers --> N
  Callers --> T
Loading

Reviews (2) · Last reviewed commit: "chore(i18n): regenerate the runtime cata..."

@pullfrog pullfrog 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.

✅ No new issues found.

Reviewed changes

  • Split out onboarding defaults — ONBOARDING_FINAL_STEP, ONBOARDING_FLOW_VERSION, and getDefaultOnboardingState moved verbatim to src/shared/onboarding-defaults.ts; all importers updated.
  • Split out notification defaults — getDefaultNotificationSettings moved to src/shared/notification-settings-defaults.ts.
  • Split out terminal platform defaults — defaultTerminalFontFamily (newly exported), getDefaultPrimarySelectionMiddleClickPaste, and getDefaultTerminalRightClickToPaste moved to src/shared/terminal-platform-defaults.ts; constants.ts imports and still calls them from getDefaultSettings.
  • Import-site updates — 29 touched files re-point to the new modules with no re-exports, keeping src/shared/constants.ts at 294 lines (under the 300-line max-lines limit).

Verified that no other import ... from '*/constants' block anywhere in the repo still pulls a moved symbol, that no barrel re-exports them, and that the new modules import only *-types so there is no cycle back to constants.ts. The moved code is byte-identical apart from the added export on defaultTerminalFontFamily, and src/shared/constants.test.ts (31 tests) passes locally.

Pullfrog  | View workflow run | Using DeepSeek Flash (free via Pullfrog for OSS) | 𝕏

@pullfrog pullfrog 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.

✅ No new issues found.

Reviewed changes

  • Moved the anti-slop shapedSidebar naming exemption from **/src/shared/constants.ts to **/src/shared/onboarding-defaults.ts in config/oxlint-anti-slop.json, following the field to its new module. The remaining import-site re-points in this delta are unchanged from the prior review.

Verified the exemption relocation is needed and sufficient: shapedSidebar now appears as a symbol only in onboarding-defaults.ts and the already-exempted onboarding-state-types.ts, while the telemetry-schema occurrence is a string literal the no-shape-in-symbol-names rule never inspects. constants.ts retains no shape identifier, so dropping its exemption is safe, and the anti-slop single-file check over the four touched shared files passes with 0 warnings and 0 errors.

Pullfrog  | View workflow run | Using DeepSeek Flash (free via Pullfrog for OSS) | 𝕏

@pullfrog pullfrog 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.

✅ No new issues found.

Reviewed changes

  • Merged main into the branch — the merge itself introduces no regression: a repo-wide scan for import ... from '*/constants' blocks that still reference a moved symbol (getDefaultOnboardingState, getDefaultNotificationSettings, ONBOARDING_*, defaultTerminalFontFamily, getDefault*Paste) returns 0 hits, so no call site added on main re-imports a moved symbol from its old location.
  • Regenerated the runtime-required English catalog — src/renderer/src/i18n/en-runtime-required.json now adds the previously-missing sparsePreset.findPath entry and restores the generator's canonical key ordering. Running the CI path (verify:localization-catalogs → generate-runtime-required-english-catalog.mjs) at head exits 0 with no missing, contradicting, or superfluous entries, and the file parses as valid JSON. This clears the "Verify localization catalogs" red check.

src/shared/constants.ts remains at 294 lines (under the 300-line max-lines limit), and the moved default builders remain byte-identical to main's copies.

Pullfrog  | View workflow run | Using DeepSeek Flash (free via Pullfrog for OSS) | 𝕏

@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: d0d77141-de58-497d-abff-cb25d59b2ed7

📥 Commits

Reviewing files that changed from the base of the PR and between 5a04dd5 and 48b09e9.

📒 Files selected for processing (1)
  • src/renderer/src/i18n/en-runtime-required.json

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

Comment on lines 2898 to 2902
"sparsePreset": {
"description": "Choose the folders a new workspace checks out.",
"findPath": "Find a folder…",
"finishEditing": "Save or cancel the preset to continue creating your workspace.",
"pathHelp": "One repository-relative directory per line. Spaces within a path are preserved."

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

Use the lookup key expected by SparseDirectoryPicker.

SparseDirectoryPicker requests sparsePreset.findOrTypePath, but this catalog adds sparsePreset.findPath. The added entry is therefore not used, so the folder-search label can remain untranslated.

Suggested fix
-    "findPath": "Find a folder…",
+    "findOrTypePath": "Find a folder…",
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
"sparsePreset": {
"description": "Choose the folders a new workspace checks out.",
"findPath": "Find a folder…",
"finishEditing": "Save or cancel the preset to continue creating your workspace.",
"pathHelp": "One repository-relative directory per line. Spaces within a path are preserved."
"sparsePreset": {
"description": "Choose the folders a new workspace checks out.",
"findOrTypePath": "Find a folder…",
"finishEditing": "Save or cancel the preset to continue creating your workspace.",
"pathHelp": "One repository-relative directory per line. Spaces within a path are preserved."

@brennanb2025
brennanb2025 merged commit 2807332 into main Sep 29, 2026
76 of 78 checks passed
brennanb2025 added a commit that referenced this pull request Sep 29, 2026
brennanb2025 added a commit that referenced this pull request Sep 29, 2026
brennanb2025 added a commit that referenced this pull request Sep 29, 2026
brennanb2025 added a commit that referenced this pull request Sep 29, 2026
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