Skip to content

Conversation

maltsev
Copy link
Member

@maltsev maltsev commented Sep 29, 2025

I believe the old code didn't handle the edge case when both options and rc.config were present.

Summary by CodeRabbit

  • Bug Fixes
    • Improved configuration handling: settings from configuration files are now consistently merged with runtime options. Runtime options continue to take precedence on conflicts, but file-based settings are no longer ignored when options are provided. This ensures more predictable behavior across environments and prevents unexpected defaults from overriding user-provided configurations.

@maltsev maltsev added the bug label Sep 29, 2025
@maltsev maltsev mentioned this pull request Sep 29, 2025
@maltsev
Copy link
Member Author

maltsev commented Oct 3, 2025

@coderabbitai review

Copy link

coderabbitai bot commented Oct 3, 2025

✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

Copy link

coderabbitai bot commented Oct 3, 2025

Walkthrough

The loadConfig function’s merging logic was updated: when a config file is found, its rc.config is always merged into restConfig, with restConfig taking precedence on conflicts. Previously, rc.config could override restConfig conditionally. Function signatures and return types remain unchanged.

Changes

Cohort / File(s) Summary
Config loading merge behavior
src/index.ts
Changed loadConfig to always merge found rc.config into restConfig using spread merge, ensuring restConfig overrides overlapping keys; removed prior conditional override behavior. No signature/type changes.

Sequence Diagram(s)

sequenceDiagram
  autonumber
  participant Caller
  participant loadConfig
  participant RC as Config File (rc)
  Note over loadConfig: Start with runtime options → restConfig

  Caller->>loadConfig: invoke(options)
  alt Config file found
    loadConfig->>RC: read rc.config
    Note over loadConfig,RC: Merge rc.config into restConfig<br/>(restConfig values take precedence)
  else No config file
    Note over loadConfig: Use restConfig as-is
  end
  loadConfig-->>Caller: return { ...restConfig, ... }
Loading

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Poem

I nibbled through configs with careful delight,
Blending file flavors with runtime’s bite.
rc whispers softly, rest stands tall—
Merge them together, no clash at all.
Thump-thump! says the code, tidy and neat,
A carrot-shaped diff, compact and sweet. 🥕

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title Check ✅ Passed The title “Fix options loading and merging” accurately summarizes the core change to the loadConfig logic by highlighting the correction of how options and configuration files are merged, and it does so in a concise, clear manner that a teammate scanning history can immediately understand.
✨ 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 config-fix

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between d33cba7 and 108e4a3.

📒 Files selected for processing (1)
  • src/index.ts (1 hunks)
🔇 Additional comments (1)
src/index.ts (1)

38-38: LGTM! Merge order correctly prioritizes runtime options.

The object spread ensures restConfig (runtime options) takes precedence over rc.config (config file options), fixing the edge case mentioned in the PR description. Note that this is a shallow merge—if module options contain nested objects, the entire nested object from restConfig will replace the one from rc.config rather than deep merging properties.


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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant