Fixes #2834 Separator Selector is styled differently and pops-under pk-sim#2835
Fixes #2834 Separator Selector is styled differently and pops-under pk-sim#2835rwmcintosh merged 2 commits intodevelopfrom
Conversation
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 42 minutes and 22 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, 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 have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe refactoring replaces direct property assignment with an explicit Changes
Sequence DiagramsequenceDiagram
participant Caller as Caller
participant DSF as DataSourceFile
participant HWM as HeavyWorkManager
participant Child as CsvDataSourceFile/<br/>ExcelDataSourceFile
Caller->>DSF: LoadFromFile(path)
activate DSF
DSF->>DSF: Path = path
DSF->>DSF: Create CancellationTokenSource
DSF->>HWM: Start(action with DoLoadWork)
activate HWM
HWM->>Child: DoLoadWork(path, token)
activate Child
Child->>Child: Read/Parse file
Child->>Child: Update DataSheets
Child-->>Child: Handle exceptions & rollback
deactivate Child
HWM-->>DSF: Complete/Suppress OperationCanceledException
deactivate HWM
DSF-->>Caller: Return
deactivate DSF
Estimated Code Review Effort🎯 3 (Moderate) | ⏱️ ~22 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 3❌ Failed checks (2 warnings, 1 inconclusive)
✅ Passed checks (1 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
src/OSPSuite.Infrastructure.Import/Core/DataSourceFileReaders/CsvDataSourceFile.cs (1)
17-17: Consider passingCSVSeparatorsinstead of storing it as a field.
_csvSeparatorsis effectively a hidden parameter toDoLoadWork. This works today becauseDoLoadWorkis only ever reached via this class'sLoadFromFileoverride, but it's fragile: any future code path that invokesbase.LoadFromFile(or a subclass ofCsvDataSourceFilethat forgets the override) will enterDoLoadWorkwith a stale or null_csvSeparatorsand NRE on_csvSeparators.ColumnSeparator/.DecimalSeparator. A cleaner shape would be to thread the separators through as parameters (e.g., an overloadDoLoadWork(string path, CSVSeparators separators, CancellationToken ct)on this class, still called from within the heavy work closure), or at minimum add a defensive null-check at the top ofDoLoadWork.Also applies to: 35-43
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/OSPSuite.Infrastructure.Import/Core/DataSourceFileReaders/CsvDataSourceFile.cs` at line 17, Do not rely on the private field `_csvSeparators` as an implicit dependency for `DoLoadWork`; instead add a `CSVSeparators separators` parameter to `DoLoadWork` (e.g., `DoLoadWork(string path, CSVSeparators separators, CancellationToken ct)`) and update `LoadFromFile` to pass the resolved separators into that call (remove the `_csvSeparators` field or stop using it), and if you prefer a minimal change add an explicit defensive null-check at the start of `DoLoadWork` that throws a clear ArgumentNullException referencing `separators`/`_csvSeparators`; update any internal calls or overrides to use the new signature so no code path can reach `DoLoadWork` with a stale or null separators instance.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/OSPSuite.Infrastructure.Import/Core/DataSourceFile.cs`:
- Around line 64-79: In LoadFromFile, don't set the Path property before running
the heavy load; currently Path = path runs before _heavyWorkManager.Start and
can remain set when DoLoadWork fails, diverging from DataSheets. Move the Path
assignment to after the call to _heavyWorkManager.Start and only assign when
Start indicates success (check its bool return value), so that Path is updated
only if the load completed successfully; keep the existing
CancellationTokenSource, the try/catch around DoLoadWork, and the same call
signature to _heavyWorkManager.Start when implementing this change.
- Around line 64-79: LoadFromFile currently calls _heavyWorkManager.Start(...)
and ignores its boolean result so exceptions thrown by DoLoadWork are swallowed;
modify LoadFromFile (the method that creates the CancellationTokenSource and
calls _heavyWorkManager.Start) to either capture any exception from the
background action (e.g. store it in a local Exception variable from inside the
closure) and rethrow it after Start returns, or check the bool return value of
_heavyWorkManager.Start and, when false, throw the appropriate
InvalidObservedDataFileException (or wrap/propagate the captured exception) so
failures in DoLoadWork surface to the caller.
In `@src/OSPSuite.UI/Views/Importer/CsvSeparatorSelectorView.cs`:
- Line 20: The WinForms designer needs a parameterless design-time constructor
for the CSVSeparatorSelectorView class; add a public parameterless constructor
CSVSeparatorSelectorView() that delegates to the existing
CSVSeparatorSelectorView(IShell shell) by calling : this(null) (or internally
calling the shell-aware constructor with null) so the partial class can be
instantiated at design time consistent with other views like ObjectBaseView.
---
Nitpick comments:
In
`@src/OSPSuite.Infrastructure.Import/Core/DataSourceFileReaders/CsvDataSourceFile.cs`:
- Line 17: Do not rely on the private field `_csvSeparators` as an implicit
dependency for `DoLoadWork`; instead add a `CSVSeparators separators` parameter
to `DoLoadWork` (e.g., `DoLoadWork(string path, CSVSeparators separators,
CancellationToken ct)`) and update `LoadFromFile` to pass the resolved
separators into that call (remove the `_csvSeparators` field or stop using it),
and if you prefer a minimal change add an explicit defensive null-check at the
start of `DoLoadWork` that throws a clear ArgumentNullException referencing
`separators`/`_csvSeparators`; update any internal calls or overrides to use the
new signature so no code path can reach `DoLoadWork` with a stale or null
separators instance.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f3f7278a-99f2-4ac3-aa3e-86049bfcb306
📒 Files selected for processing (9)
src/OSPSuite.Infrastructure.Import/Core/DataSourceFile.cssrc/OSPSuite.Infrastructure.Import/Core/DataSourceFileParser.cssrc/OSPSuite.Infrastructure.Import/Core/DataSourceFileReaders/CsvDataSourceFile.cssrc/OSPSuite.Infrastructure.Import/Core/DataSourceFileReaders/ExcelDataSourceFile.cssrc/OSPSuite.Presentation/Presenters/Importer/ImporterDataPresenter.cssrc/OSPSuite.UI/Views/Importer/CsvSeparatorSelectorView.cstests/OSPSuite.Presentation.Tests/Importer/Core/DataSourceFileReaders/CsvDataSourceFileSpecs.cstests/OSPSuite.Presentation.Tests/Importer/Core/DataSourceFileReaders/ExcelDataSourceFileSpecs.cstests/OSPSuite.Presentation.Tests/Importer/Presenters/ImporterPresenterSpecs.cs
| public override void LoadFromFile(string path) | ||
| { | ||
| var csvSeparators = _csvSeparatorSelector.GetCsvSeparator(path); | ||
| _csvSeparators = _csvSeparatorSelector.GetCsvSeparator(path); |
There was a problem hiding this comment.
This opens a modal dialog and cannot be done from within HeavyWorkManager
2 participants
Fixes #2834 Separator Selector is styled differently and pops-under pk-sim
Description
We were opening a new dialog from within the heavyworkmanager and not setting the parent handle
Type of change
Please mark relevant options with an
xin the brackets.How Has This Been Tested?
Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration
Reviewer checklist
Mark everything that needs to be checked before merging the PR.
This change requires a documentation updateabove is selectedScreenshots (if appropriate):
Questions (if appropriate):
Summary by CodeRabbit
Bug Fixes
Refactor