Add question bank merge support#29
Conversation
|
Warning Review limit reached
More reviews will be available in 33 minutes and 13 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThis PR adds comprehensive multi-file question bank merge functionality. The core service layer implements format detection for Ksb and Mtb Excel formats, multi-file reading with optional deduplication, and merge APIs. The CLI exposes a ChangesQuestion Bank Merge Feature
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
|
✅ Code Coverage Report
🚀 Keep up the good work! |
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
ModerBox.Cli/Commands/QuestionBankCommand.cs (1)
40-46:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFix duplicate option aliases between
question-bankandmerge.
question-bankdefines required options--source/-sand--target/-t, while themergesubcommand also defines--sources/-sand--target/-t. In System.CommandLine, option aliases must be unique in the command tree—redeclaring the same aliases leads to parser/grammar validation errors (e.g., “Alias '--duplicate' is already in use.”). Moving the convert-only options off the parent (so the parent is a container, with the conversion flow behind its own subcommand) removes the alias collisions and keepsmergeunambiguous.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ModerBox.Cli/Commands/QuestionBankCommand.cs` around lines 40 - 46, The parent Command("question-bank") currently registers convert-specific options (sourceOption, targetOption, sourceFormatOption, targetFormatOption) which conflict with the merge subcommand's aliases; remove these options from the parent and instead create a dedicated "convert" subcommand (e.g., Command("convert") or "convert-to") that registers sourceOption, targetOption, sourceFormatOption and targetFormatOption, then call command.AddCommand(CreateMergeCommand()) and command.AddCommand(convertCommand) so the parent acts only as a container and alias collisions with CreateMergeCommand() are avoided.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@ModerBox.Cli/README.md`:
- Around line 125-133: The fenced code block containing the command starting
with "qb merge --sources" is missing a language identifier; update the opening
fence from ``` to ```bash so the block becomes a bash code block (i.e., add
"bash" after the opening backticks) to satisfy markdownlint and enable proper
syntax highlighting for the snippet.
In `@ModerBox/ViewModels/QuestionBankConversionViewModel.cs`:
- Around line 412-413: The call to _llmService.GenerateAnalysisAsync currently
passes sourcePaths[0] which becomes the persisted progress key; when isMerge is
true this can collide with single-file runs or other merges. Change the call to
compute a deterministic merge-specific key (e.g., hash or join of the full
sourcePaths list) when isMerge==true and pass that key instead of
sourcePaths[0]; update any variable passed to GenerateAnalysisAsync so
GenerateAnalysisAsync receives the full-source merge key for merged runs while
non-merge runs continue to pass the single-file key.
---
Outside diff comments:
In `@ModerBox.Cli/Commands/QuestionBankCommand.cs`:
- Around line 40-46: The parent Command("question-bank") currently registers
convert-specific options (sourceOption, targetOption, sourceFormatOption,
targetFormatOption) which conflict with the merge subcommand's aliases; remove
these options from the parent and instead create a dedicated "convert"
subcommand (e.g., Command("convert") or "convert-to") that registers
sourceOption, targetOption, sourceFormatOption and targetFormatOption, then call
command.AddCommand(CreateMergeCommand()) and command.AddCommand(convertCommand)
so the parent acts only as a container and alias collisions with
CreateMergeCommand() are avoided.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: dc770182-a949-42d1-b34f-f06f7c835860
📒 Files selected for processing (10)
ModerBox.Cli.Test/CommandTests.csModerBox.Cli/Commands/QuestionBankCommand.csModerBox.Cli/README.mdModerBox.MCP/Tools/QuestionBankTools.csModerBox.QuestionBank.Test/QuestionBankServiceTests.csModerBox.QuestionBank/README.mdModerBox.QuestionBank/Services/QuestionBankConversionService.csModerBox/ViewModels/QuestionBankConversionViewModel.csModerBox/Views/UserControls/QuestionBankConversion.axamlREADME.md
| [McpServerTool, Description("Merge multiple question bank files into one output file. Supports auto-detection per source file and optional duplicate removal.")] | ||
| public static QuestionBankMergeResult MergeQuestionBanks( | ||
| [Description("Source question bank file paths to merge")] string[] sourcePaths, | ||
| [Description("Target output file path")] string targetPath, | ||
| [Description("Target format (Ksb, Mtb, Wldx, Wldx4, Xiaobao, XiaobaoTxt)")] QuestionBankTargetFormat targetFormat, | ||
| [Description("Source format (AutoDetect, Txt, Ksb, Mtb, Wldx, Wldx4, Exc, Gdpx, Simple). If not specified, each source file will be auto-detected.")] QuestionBankSourceFormat? sourceFormat = null, | ||
| [Description("Remove duplicate questions while merging. Defaults to true.")] bool deduplicate = true) | ||
| { | ||
| var result = new QuestionBankMergeResult | ||
| { | ||
| SourcePaths = sourcePaths, | ||
| TargetPath = targetPath, | ||
| TargetFormat = targetFormat.ToString() | ||
| }; | ||
|
|
||
| try | ||
| { | ||
| if (sourcePaths.Length == 0) | ||
| { | ||
| result.Success = false; | ||
| result.ErrorMessage = "At least one source file is required."; | ||
| return result; | ||
| } | ||
|
|
||
| var missing = sourcePaths.FirstOrDefault(path => !File.Exists(path)); | ||
| if (missing is not null) | ||
| { | ||
| result.Success = false; | ||
| result.ErrorMessage = $"Source file does not exist: {missing}"; | ||
| return result; | ||
| } | ||
|
|
||
| var service = new QuestionBankConversionService(); | ||
| var sourceFmt = sourceFormat ?? QuestionBankSourceFormat.AutoDetect; | ||
| var title = Path.GetFileNameWithoutExtension(targetPath); | ||
|
|
||
| var summary = service.Merge(sourcePaths, targetPath, sourceFmt, targetFormat, deduplicate, title); | ||
|
|
||
| result.Success = true; | ||
| result.SourceFileCount = summary.SourceFileCount; | ||
| result.TotalQuestionCount = summary.TotalQuestionCount; | ||
| result.DuplicateQuestionCount = summary.DuplicateQuestionCount; | ||
| result.OutputQuestionCount = summary.OutputQuestionCount; | ||
| result.TargetFormat = summary.TargetFormat.ToString(); | ||
| result.OutputPath = summary.TargetPath; | ||
| result.Sources = summary.Sources | ||
| .Select(source => new QuestionBankMergeSourceResult | ||
| { | ||
| SourcePath = source.SourcePath, | ||
| SourceFormat = source.SourceFormat.ToString(), | ||
| QuestionCount = source.QuestionCount | ||
| }) | ||
| .ToArray(); | ||
| } | ||
| catch (Exception ex) | ||
| { | ||
| result.Success = false; | ||
| result.ErrorMessage = ex.Message; | ||
| } | ||
|
|
||
| return result; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate the target file
ls -la ModerBox.MCP/Tools/QuestionBankTools.cs
# Show the relevant section with line numbers
sed -n '1,220p' ModerBox.MCP/Tools/QuestionBankTools.cs | nl -ba | sed -n '60,190p'
# Search for async tool methods in same file and attribute usage patterns
rg -n --context 2 "\[McpServerTool" ModerBox.MCP/Tools/QuestionBankTools.cs
rg -n "public (static )?(async )?" ModerBox.MCP/Tools/QuestionBankTools.cs
# Check if any other tool methods in ModerBox.MCP/Tools are async-first (Task-returning)
rg -n "Task<.*>\s+.*\(" ModerBox.MCP/Tools -S --glob '*.cs'
rg -n "async\s+Task<" ModerBox.MCP/Tools -S --glob '*.cs'Repository: ModerRAS/ModerBox
Length of output: 211
🏁 Script executed:
#!/bin/bash
set -euo pipefail
ls -la ModerBox.MCP/Tools/QuestionBankTools.cs
# Show method region around the reported lines
sed -n '1,240p' ModerBox.MCP/Tools/QuestionBankTools.cs | nl -ba | sed -n '70,170p'
# Confirm current signature (sync vs Task) and attribute usage
rg -n "MergeQuestionBanks" ModerBox.MCP/Tools/QuestionBankTools.cs
rg -n "\[McpServerTool" ModerBox.MCP/Tools/QuestionBankTools.cs
# Check for async-first patterns elsewhere in MCP tools
rg -n "async\s+Task<" ModerBox.MCP/Tools --glob '*.cs'
rg -n "Task<[^>]+>\s+public.*\(" ModerBox.MCP/Tools --glob '*.cs'
rg -n "public\s+static\s+[A-Za-z0-9_<>]+\s+MergeQuestionBanks\s*\(" ModerBox.MCP/Tools --glob '*.cs'Repository: ModerRAS/ModerBox
Length of output: 211
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Confirm file exists
ls -la ModerBox.MCP/Tools/QuestionBankTools.cs
# Show the reported region with line numbers (no nl available)
cat -n ModerBox.MCP/Tools/QuestionBankTools.cs | sed -n '70,180p'
# Locate MergeQuestionBanks definition + attributes
rg -n "MergeQuestionBanks" ModerBox.MCP/Tools/QuestionBankTools.cs
rg -n "\\[McpServerTool" ModerBox.MCP/Tools/QuestionBankTools.cs
rg -n "Description\\(" ModerBox.MCP/Tools/QuestionBankTools.cs
# Scan other MCP tools for async-first patterns (Task-returning / async Task)
rg -n --glob '*.cs' "async\s+Task<" ModerBox.MCP/Tools
rg -n --glob '*.cs' "Task<[^>]+>\s+public\s+.*\(" ModerBox.MCP/Tools
rg -n --glob '*.cs' "public\s+static\s+.*\s+MergeQuestionBanks\s*\(" ModerBox.MCP/ToolsRepository: ModerRAS/ModerBox
Length of output: 9223
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find QuestionBankConversionService definition
rg -n "class QuestionBankConversionService" -S . --glob '*.cs'
rg -n "QuestionBankConversionService" ModerBox.MCP/Tools/QuestionBankTools.cs
# Locate Merge implementation
rg -n "Merge\s*\(" -S . --glob '*.cs' | rg "QuestionBankConversionService|MergeQuestion" || true
rg -n "QuestionBankConversionService\s*\.\s*Merge|service\.Merge" -S . --glob '*.cs'
# Print the Merge method implementation if found
rg -n "MergeQuestionBanks|Merge\(" -S . --glob '*.cs' --files-with-matches | head -n 50Repository: ModerRAS/ModerBox
Length of output: 1809
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file="ModerBox.QuestionBank/Services/QuestionBankConversionService.cs"
# Show the Merge method + any related async patterns around it
sed -n '180,360p' "$file" | cat -n
# Search within the service file for async keywords
rg -n "async\s+Task|Task<|ValueTask" "$file"Repository: ModerRAS/ModerBox
Length of output: 7946
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file="ModerBox.MCP/Tools/QuestionBankTools.cs"
# Show top of file with attributes/class/method signatures
sed -n '1,120p' "$file" | cat -n
# Show any other tool method signatures in this file
rg -n "\\[McpServerTool" "$file"
rg -n "public\\s+static\\s+(async\\s+)?(Task<)?\\s*[^\\s]+\\s+\\w+\\s*\\(" "$file"Repository: ModerRAS/ModerBox
Length of output: 6483
Make MergeQuestionBanks async to avoid blocking MCP threads.
In ModerBox.MCP/Tools/QuestionBankTools.cs (lines ~87-148), MergeQuestionBanks is synchronous (public static QuestionBankMergeResult ...) and does full disk I/O + merge/write inline via QuestionBankConversionService.Merge(...). Other MCP tools here use public static async Task<...>, so update this tool to return Task<QuestionBankMergeResult> and implement async-first execution (make the service async or offload the work with await).
Also, ConvertQuestionBank and DetectQuestionBankFormat in the same file are currently synchronous and likely need the same async-first treatment to match the guideline.
Summary\n- add multi-file question bank merge with optional deduplication\n- expose merge flow in CLI, MCP, and Avalonia UI\n- extend source detection to KSB and MTB exports\n\n## Verification\n- dotnet test ModerBox.QuestionBank.Test/ModerBox.QuestionBank.Test.csproj\n- dotnet test ModerBox.Cli.Test/ModerBox.Cli.Test.csproj\n- dotnet build ModerBox.MCP/ModerBox.MCP.csproj\n- dotnet build ModerBox/ModerBox.csproj
Summary by CodeRabbit
Release Notes
New Features
question-bank mergecommand with support for multiple sources, customizable target format, and deduplication controlDocumentation