wip#1
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 410ab1cf2d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
410ab1c to
f70f1b0
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f70f1b052c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| return { | ||
| smdaHeaders: [...headersByUuid.values()], | ||
| isPending: results.some((result) => result.isPending), |
There was a problem hiding this comment.
When a project has multiple SMDA fields and one well_headers query errors while another succeeds, this combine still returns the successful subset with isPending: false; SmdaMappings only disables suggestion generation on wellHeadersPending, so users can generate and save matches against an incomplete SMDA target list after a field-specific outage or 422/503. Track result.isError/failed fields and block or surface the error instead of enabling suggestions with partial headers.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Summary
- Updated
useSmdaWellHeadersto include a combinedisErrorflag whenever any field-specific SMDA well-header query fails, preventing partial success from being treated as fully usable. frontend/src/services/smda.tsL77-L105 - Propagated the SMDA header error state from the wellbore mappings overview into the SMDA mappings action component. frontend/src/components/project/wellboreMappings/Overview.tsxL34-L43 frontend/src/components/project/wellboreMappings/Overview.tsxL94-L104
- Added
wellHeadersErrorto the SMDA mappings props contract. frontend/src/components/project/wellboreMappings/types.tsL79-L89 - Disabled “Suggest SMDA names” when any SMDA header lookup errors and added tooltip messaging to explain the incomplete SMDA target list. frontend/src/components/project/wellboreMappings/SmdaMappings.tsxL365-L393
- Committed the changes on the current branch with commit
ef358a0(fix: block SMDA suggestions on header errors) and created a PR titledfix: block SMDA suggestions on header load errors.
Testing
- ✅
pnpm --dir frontend exec tsc -b --pretty false ⚠️ pnpm --dir frontend lint(environment limitation: the script referencestools/biome, which is not present)⚠️ pnpm --dir frontend exec biome check . && pnpm --dir frontend exec eslint .(environment limitation:biomeis not installed)
There was a problem hiding this comment.
Summary
- Committed the requested SMDA header-error handling change as
0f66199(fix: block SMDA suggestions on header errors). - Updated
useSmdaWellHeadersto surface an aggregateisErrorvalue when any field-specific SMDA well-header query fails. frontend/src/services/smda.tsL77-L105 - Propagated the SMDA header error state from
OverviewintoSmdaMappings. frontend/src/components/project/wellboreMappings/Overview.tsxL33-L43 frontend/src/components/project/wellboreMappings/Overview.tsxL94-L104 - Added
wellHeadersErrorto the SMDA mappings props contract. frontend/src/components/project/wellboreMappings/types.tsL79-L89 - Disabled “Suggest SMDA names” when any SMDA header lookup errors and added tooltip text explaining that not all SMDA wellbore names could be loaded. frontend/src/components/project/wellboreMappings/SmdaMappings.tsxL217-L228 frontend/src/components/project/wellboreMappings/SmdaMappings.tsxL366-L391
- Created a PR titled
fix: block SMDA suggestions on header load errors.
Testing
- ✅
pnpm --dir frontend exec tsc -b --pretty false ⚠️ pnpm --dir frontend lint— failed becausetools/biomeis not present in this environment.
| if (simulatorName) { | ||
| updated.push({ |
There was a problem hiding this comment.
When an RMS well already has multiple simulator rows, editing one row and entering a simulator name that exists on another row removes only the original row and then unconditionally appends the new mapping here. That leaves duplicate simulator mappings in mappings.json, and createWellboreMappingRows will render duplicate rows for the same RMS/simulator pair. Skip the append when the remaining mappings already contain this simulator target.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
@codex is this a valid comment? An RMS well cannot have multiple simulator rows, it will fail API validation
There was a problem hiding this comment.
Answer
I would not consider that comment valid as written.
The codebase appears to intentionally support multiple simulator mappings/rows for a single RMS well:
createWellboreMappingRowsgroups simulator mappings bysource_idinto an array, then renders one table row per simulator mapping for the same RMS well. frontend/src/components/project/wellboreMappings/functions.tsL65-L83 frontend/src/components/project/wellboreMappings/functions.tsL93-L104- The generated API/client type for
InternalWellboreMappingsis an array of mapping objects, with no client-side uniqueness constraint shown that would restrict one RMS well to exactly one simulator mapping. frontend/src/client/types.gen.tsL245-L263 - The UI copy also talks about selecting “a row” and editing its simulator/SMDA names, consistent with the table having potentially multiple rows per RMS well. frontend/src/components/project/wellboreMappings/WellboreMappingsTable.tsxL299-L304
However, the underlying review concern is still valid if the issue is duplicate mappings for the same RMS/simulator pair. In updateWellboreMapping, the function removes only the first matching original simulator mapping and then unconditionally appends the edited simulator mapping when simulatorName is non-empty. frontend/src/components/project/wellboreMappings/functions.tsL142-L165 That can create a duplicate if another remaining row for the same RMS well already has the target simulator name. frontend/src/components/project/wellboreMappings/functions.tsL148-L172
So I’d rephrase the comment to something like:
This should avoid adding a duplicate RMS-to-simulator mapping for the same RMS well and simulator name. Multiple simulator rows per RMS well appear to be supported, but duplicate rows for the same RMS/simulator pair can be produced here because the code removes only the edited row and then always appends the new simulator target.
Testing / checks
- ✅
find .. -name AGENTS.md -print && git status --short && sed -n '130,190p' frontend/src/components/project/wellboreMappings/functions.ts - ✅
sed -n '1,140p' frontend/src/components/project/wellboreMappings/functions.ts && rg "updateWellboreMapping" -n - ✅
sed -n '240,270p' frontend/src/client/types.gen.ts && sed -n '465,540p' frontend/src/client/schemas.gen.ts && sed -n '190,370p' frontend/src/components/project/wellboreMappings/functions.ts - ✅
rg "simulator" frontend/src/components/project/wellboreMappings -n | head -80 && find frontend/src -name '*test*' -o -name '*spec*' | head -50 - ✅
nl -ba frontend/src/components/project/wellboreMappings/functions.ts | sed -n '63,110p;138,174p' && nl -ba frontend/src/client/types.gen.ts | sed -n '242,264p' && nl -ba frontend/src/components/project/wellboreMappings/WellboreMappingsTable.tsx | sed -n '296,306p'
There was a problem hiding this comment.
@codex It will fail this validation https://github.com/equinor/fmu-settings/blob/main/src/fmu/settings/models/mappings.py#L369-L386
There was a problem hiding this comment.
Summary
- Updated
updateWellboreMappingso editing a wellbore row removes all existing primary RMS-to-simulator mappings for that RMS well before appending the edited simulator mapping, preventing saves that violate the API’s one-cross-system-mapping-per-target-system validation. - Kept the edited simulator append path unchanged after the cleanup, so a non-empty simulator name still writes one replacement primary simulator mapping.
- Committed the change on the current branch with commit
497828c(fix: replace existing simulator mapping) and created a PR titledfix: replace existing wellbore simulator mappings.
Testing
- ✅
pnpm --dir frontend exec tsc -b --pretty false ⚠️ pnpm --dir frontend lint— failed becausetools/biomeis not present in this environment.
1 participant
Resolves #issue
Briefly describe changes here, e.g. "Added a precise description about the
changes made to the code, and what the PR is solving."
Checklist
--cov=src/ --cov-report term-missing)