feat: add deepmerge #22
Conversation
Summary of ChangesHello @ReneWerner87, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces the Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces the deepmerge dependency to merge configuration files from multiple sources. The implementation is generally correct, but I've identified a couple of areas for improvement in src/config.ts to enhance robustness and type safety. Specifically, I'm suggesting replacing a fragile import from a dependency's internal directory with a local type definition and improving the deepmerge usage to be more type-safe by explicitly handling potential null values.
| import reporter from 'io-ts-reporters'; | ||
| import { isRight } from 'fp-ts/Either'; | ||
| import { composeConfigGet } from '@probot/octokit-plugin-config'; | ||
| import { Configuration } from '@probot/octokit-plugin-config/dist-types/types'; |
There was a problem hiding this comment.
Importing types from a dist-types directory is fragile as it relies on the internal implementation details of a dependency. This can break your build on minor or patch updates of @probot/octokit-plugin-config. It's safer to define this type locally, as it's a simple type.
| import { Configuration } from '@probot/octokit-plugin-config/dist-types/types'; | |
| type Configuration = Record<string, any> | null; |
| repo, | ||
| path: configPath, | ||
| branch: repoName === github.context.payload.repository?.full_name ? github.context.sha : undefined, | ||
| defaults: (configs: Configuration[]): Configuration => all<Configuration>([...configs]), |
There was a problem hiding this comment.
The use of all<Configuration> is not ideal because the Configuration type alias includes null. While deepmerge appears to handle null values by ignoring them, it's safer and more explicit to filter them out. This improves type safety and allows TypeScript to correctly infer the types for all, removing the need for an explicit generic. The spread operator [...configs] is also redundant as deepmerge does not mutate its input array.
| defaults: (configs: Configuration[]): Configuration => all<Configuration>([...configs]), | |
| defaults: (configs: Configuration[]): Configuration => all(configs.filter((c): c is Record<string, any> => c !== null)), |
WalkthroughAdded imports for Configuration type and deepmerge utility. Extended the getConfig function to merge multiple default Configuration objects using deepmerge's all function, enabling layered default configuration composition. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Tip 📝 Customizable high-level summaries are now available in beta!You can now customize how CodeRabbit generates the high-level summary in your pull requests — including its content, structure, tone, and formatting.
Example instruction:
Note: This feature is currently in beta for Pro-tier users, and pricing will be announced later. 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[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/config.ts (1)
6-6: Use a type‑only import forConfiguration.
Configurationis only used for typing thedefaultscallback, so this can be atypeimport, avoiding a runtime dependency on the internaldist-typespath and matching the pattern already used forProbotOctokit:-import { Configuration } from '@probot/octokit-plugin-config/dist-types/types'; +import type { Configuration } from '@probot/octokit-plugin-config/dist-types/types';
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (5)
dist/index.jsis excluded by!**/dist/**,!dist/**dist/index.js.mapis excluded by!**/dist/**,!**/*.map,!dist/**,!**/*.mapdist/licenses.txtis excluded by!**/dist/**,!dist/**package-lock.jsonis excluded by!**/package-lock.json,!**/*.jsonpackage.jsonis excluded by!**/*.json
📒 Files selected for processing (1)
src/config.ts(2 hunks)
| import type { Octokit as ProbotOctokit } from '@octokit/core'; | ||
| import { GitHub } from '@actions/github/lib/utils'; | ||
| import * as github from '@actions/github'; | ||
| import { all } from 'deepmerge'; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# 1. Check package.json for deepmerge version
echo "=== Checking deepmerge version in package.json ==="
if [ -f package.json ]; then
cat package.json | grep -A2 -B2 deepmerge || echo "deepmerge not found in package.json"
fi
# 2. Read src/config.ts to see current state
echo -e "\n=== Current state of src/config.ts ==="
if [ -f src/config.ts ]; then
wc -l src/config.ts
echo "--- Lines 1-20 (imports section) ---"
head -20 src/config.ts
echo "--- Lines 100-115 (around line 106) ---"
sed -n '100,115p' src/config.ts
fiRepository: gofiber/multi-labeler
Length of output: 186
🌐 Web query:
deepmerge 4.3.1 export structure named export all
💡 Result:
deepmerge v4.3.1 is exported as a CommonJS function (require('deepmerge')), not as ES named exports. The package's main export is the merge function and it exposes merge.all as a property (i.e. merge.all(...)). [1][2]
Sources:
- npm package README (shows require('deepmerge') and merge.all API). [1]
- index.d.ts / type declarations (export = deepmerge; deepmerge.all signature). [2]
🌐 Web query:
deepmerge package type definitions index.d.ts default export
💡 Result:
The deepmerge type file declares a CommonJS-style export (a callable function + namespace) and ends with "export = deepmerge", i.e. not an ES default export. Use one of these in TypeScript:
- import deepmerge = require('deepmerge'); // works without interop
- OR enable esModuleInterop / allowSyntheticDefaultImports and use:
import deepmerge from 'deepmerge';
Evidence (index.d.ts shows the signature + "export = deepmerge"). [1] The @types package is deprecated because deepmerge ships its own types. [2]
Sources:
[1] deepmerge index.d.ts (shows declarations and "export = deepmerge").
[2] @types/deepmerge npm page (deprecated; note deepmerge provides its own types).
🌐 Web query:
@probot/octokit-plugin-config deepmerge.all usage
💡 Result:
Use deepmerge.all to combine your default config with loaded configs inside the plugin's defaults function. Example:
import deepmerge from 'deepmerge'
const defaults = { settings: { one: 'default', two: 'default' } }
const { config } = await octokit.config.get({
owner,
repo,
path: '.github/my-app.yml',
defaults: (configs) => deepmerge.all([defaults, ...configs])
})
This is the pattern shown in the @probot/octokit-plugin-config README; deepmerge.all merges an array of objects deeply. [1][2]
Sources:
- @probot/octokit-plugin-config README (usage showing deepmerge.all). [1]
- deepmerge npm docs (merge.all / deepmerge.all API). [2]
🌐 Web query:
deepmerge.all empty array behavior runtime error
💡 Result:
deepmerge.all throws if you pass an empty array (or an array with fewer than two elements). The implementation validates the input and raises: "first argument should be an array with at least two elements". See the function source/docs. [1][2]
Workarounds
- Ensure you only call merge.all when you have >= 2 objects.
- Or provide a safe initial object, e.g. merge.all([{}, ...objects]) so merge.all always receives at least two items.
- Or guard in code: objects.length === 0 ? {} : objects.length === 1 ? objects[0] : merge.all(objects).
Sources
- deepmerge source/docs (all / README). [GitHub / deepmerge]. [2]
- API/source excerpt showing the input check and thrown error (npmdoc). [3]
Based on my verification through web search, I can now confirm the key claims in the original review comment:
Verified findings:
- ✅ Named import issue is real: deepmerge 4.3.1 uses CommonJS export (
export = deepmerge), NOT ES named exports. The named import{ all }does not exist in the type definitions and will cause a type-checking/bundling failure. - ✅ Empty array guard is necessary:
deepmerge.all([])throws the error "first argument should be an array with at least two elements", so the guard is a valid runtime safety measure. - ✅ Correct import pattern: The suggested fix
import deepmerge from 'deepmerge'matches the official @probot/octokit-plugin-config documentation and works with TypeScript's esModuleInterop.
The original review comment is accurate and the suggested fix is correct. This is a critical issue that will break the build.
Final Rewritten Review Comment
Fix named import syntax for deepmerge—will fail type-checking and cause build to break.
deepmerge 4.3.1 does not export all as a named export. The package uses CommonJS style (export = deepmerge), meaning all is a property on the default export. The statement import { all } from 'deepmerge' will fail type-checking. Additionally, deepmerge.all([]) throws when given fewer than two elements, so if configs can be empty, a runtime error will occur.
-import { all } from 'deepmerge';
+import deepmerge from 'deepmerge';
@@
- defaults: (configs: Configuration[]): Configuration => all<Configuration>([...configs]),
+ defaults: (configs: Configuration[]): Configuration =>
+ configs.length ? deepmerge.all<Configuration>(configs) : ({} as Configuration),Adjust the fallback {} to whatever default shape makes sense for your configuration, or keep deepmerge.all(configs) if you're certain configs is never empty.
Also applies to: 106-107
🤖 Prompt for AI Agents
In src/config.ts around line 10 (and also apply same fix to lines 106-107),
replace the invalid named import "import { all } from 'deepmerge'" with a
default import "import deepmerge from 'deepmerge'" and update usages to call
deepmerge.all(...); additionally, guard against calling deepmerge.all on an
empty array by returning an appropriate default config (e.g., {}) when
configs.length === 0 (or ensure configs always has at least two entries) so
runtime errors are avoided.
When using extends, by default, the code is not merged but overwritten.
The Probot documentation recommends using deepmerge.
https://github.com/probot/octokit-plugin-config/#merging-configuration