feat(workbench): various UX improvements#8165
Open
grantfitzsimmons wants to merge 44 commits into
Open
Conversation
Triggered by e922695 on branch refs/heads/issue-8153
Update terminology and user-facing copy from 'Upload Plan' to 'Mapping' (or 'Mapping Plan') across the WorkBench.
📝 WalkthroughWalkthroughThis PR improves the WorkBench upload experience and fixes browser compatibility issues by adding new icons, refining dialog styling, enhancing the upload confirmation flow with record count breakdowns, introducing a plan selection feature, and updating terminology and localization strings across the interface. ChangesWorkBench UI improvements and icon system
Suggested reviewers
🚥 Pre-merge checks | ✅ 5 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (5 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 |
grantfitzsimmons
changed the title
![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 (5)
specifyweb/frontend/js_src/lib/localization/workbench.ts (1)
980-989: 💤 Low valueConsider following the localization workflow for new keys.
The new
metadatakey includes translations for all locales. Based on learnings, new localization keys should typically include only theen-usentry in code, with other locales populated via Weblate to maintain the translation workflow.This doesn't affect functionality, but keeping the workflow consistent helps maintain translation quality and attribution. Based on learnings from previous PR feedback, it's preferred to let Weblate handle non-English translations for new keys.
🤖 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 `@specifyweb/frontend/js_src/lib/localization/workbench.ts` around lines 980 - 989, The new localization object key metadata currently includes translations for many locales; remove all non-English entries and leave only the 'en-us' value for metadata so new keys follow the Weblate workflow (i.e., keep metadata: { 'en-us': 'Metadata' } and remove 'de-ch','es-es','fr-fr','ru-ru','uk-ua','pt-br','hr-hr') so translations are later added via Weblate.Source: Learnings
specifyweb/frontend/js_src/lib/localization/wbPlan.ts (1)
807-826: 💤 Low valueNew localization keys include translations for all locales.
Per the project's localization guidelines, new keys should typically include only the
en-usentry in code, with other locales populated via Weblate. TheinvalidJsonFileandinvalidJsonFileDescriptionkeys (Lines 807-826) include translations for all locales. This may be intentional if these were pre-translated or part of a batch update, but it's worth confirming this aligns with the localization workflow.Based on learnings, new localization keys should include only the 'en-us' entry in code, with other locales populated via Weblate.
🤖 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 `@specifyweb/frontend/js_src/lib/localization/wbPlan.ts` around lines 807 - 826, Remove the inline translations for all locales and leave only the 'en-us' entries for the two new localization keys so they follow the project's Weblate workflow: update the invalidJsonFile and invalidJsonFileDescription objects to contain only the 'en-us' string values (e.g. 'The selected file is not valid JSON.' and 'Please select a valid JSON data set mapping file.' respectively), removing the other locale keys (ru-ru, es-es, fr-fr, uk-ua, de-ch, pt-br, hr-hr) so translators will add them via Weblate.Source: Learnings
specifyweb/frontend/js_src/lib/components/WbToolkit/DevShowPlan.tsx (1)
85-99: ⚖️ Poor tradeoffConsider adding schema validation for imported JSON.
The import handler validates that the file contains valid JSON syntax (Line 93), but does not verify that the JSON structure matches the
UploadPlanschema. Invalid structure will only be caught when the user tries to save. Adding client-side schema validation would provide earlier feedback.💡 Optional schema validation approach
You could add a basic structure check:
.then((text) => { const parsed = JSON.parse(text); // Basic structure check if (!parsed.baseTableName || !parsed.uploadable) { throw new Error('Invalid mapping structure'); } setUploadPlane(text); })However, full schema validation would be more complex and may not be worth the effort since the backend validates on save.
🤖 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 `@specifyweb/frontend/js_src/lib/components/WbToolkit/DevShowPlan.tsx` around lines 85 - 99, handleFileSelected currently only checks JSON syntax via JSON.parse in the fileToText promise chain but does not validate the object shape against the expected UploadPlan schema; update the promise chain in handleFileSelected to parse the text into an object, perform a schema/shape check (e.g., verify required fields and types expected by UploadPlan such as baseTableName and uploadable or use a lightweight validator), and only call setUploadPlane(text) when the validation passes; on validation failure call showInvalid (or throw) so the existing catch handles it; reference handleFileSelected, fileToText, setUploadPlane, UploadPlan and showInvalid when making the change.specifyweb/frontend/js_src/lib/components/WbPlanView/State.tsx (1)
101-101: 💤 Low valueConsider avoiding the
!importantmodifier if possible.The
!w-1/3class uses Tailwind's!modifier to force override thenormalContainerwidth. While this works, it may indicate a layout conflict. IfnormalContainersets a width that needs overriding frequently, consider creating a dedicated dialog size variant instead of using!important.🤖 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 `@specifyweb/frontend/js_src/lib/components/WbPlanView/State.tsx` at line 101, The container class string in State.tsx currently uses the Tailwind forced modifier `!w-1/3` to override dialogClassNames.normalContainer; remove the `!` override and instead add a dedicated size variant (e.g., dialogClassNames.smallContainer or dialogClassNames.normalContainerSmall) in the dialogClassNames definitions and use that variant here, or adjust normalContainer to accept size variants so you can replace `${dialogClassNames.normalContainer} !w-1/3 max-h-[90%]` with a non-forced combination that references the new variant (keep the rest like max-h-[90%] unchanged).specifyweb/frontend/js_src/lib/components/WbActions/index.tsx (1)
235-272: ⚡ Quick winConsider extracting the result-type label mapping to reduce duplication.
The label mapping logic (lines 253-259) is duplicated from
WbUpload.tsxlines 166-172. Consider extracting this to a shared helper function that maps result types to localized strings.♻️ Example refactor
Create a helper function in a shared location:
function getResultTypeLabel(resultType: keyof RecordCounts): LocalizedString { return resultType === 'Uploaded' ? wbText.recordsCreated() : resultType === 'Updated' ? wbText.recordsUpdated() : resultType === 'Deleted' ? wbText.recordsDeleted() : wbText.recordsMatchedAndChanged(); }Then use it in both files:
- {resultType === 'Uploaded' - ? wbText.recordsCreated() - : resultType === 'Updated' - ? wbText.recordsUpdated() - : resultType === 'Deleted' - ? wbText.recordsDeleted() - : wbText.recordsMatchedAndChanged()} + {getResultTypeLabel(resultType)}🤖 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 `@specifyweb/frontend/js_src/lib/components/WbActions/index.tsx` around lines 235 - 272, The result-type label mapping in WbActions (the ternary block that chooses between wbText.recordsCreated/Updated/Deleted/MatchedAndChanged) is duplicated with WbUpload; extract it into a shared helper (e.g., getResultTypeLabel) that accepts the resultType (keyof RecordCounts) and returns the localized string using wbText, then import and call getResultTypeLabel(resultType) in both WbActions/index.tsx and WbUpload.tsx to replace the inline ternary; ensure the helper uses the same RecordCounts key type and wbText references so behavior and localization remain identical.
🤖 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 `@specifyweb/frontend/js_src/lib/components/WbActions/WbNoUploadPlan.tsx`:
- Around line 64-79: The TemplateSelection onSelect handler calls ping(...)
wrapped by loading(...) but lacks explicit error handling; update the onSelect
callback in WbNoUploadPlan (the TemplateSelection -> onSelect handler) to attach
a .catch handler to the ping promise so failures are handled explicitly: keep
the redirect to /specify/workbench/plan/${datasetId}/ only on success, and in
.catch display a user-friendly error (e.g., via existing UI toast/alert function
or set state to show an error), stop/clear any loading state, and include the
error details for debugging; ensure the success path still calls
window.location.href and the error path does not redirect.
---
Nitpick comments:
In `@specifyweb/frontend/js_src/lib/components/WbActions/index.tsx`:
- Around line 235-272: The result-type label mapping in WbActions (the ternary
block that chooses between
wbText.recordsCreated/Updated/Deleted/MatchedAndChanged) is duplicated with
WbUpload; extract it into a shared helper (e.g., getResultTypeLabel) that
accepts the resultType (keyof RecordCounts) and returns the localized string
using wbText, then import and call getResultTypeLabel(resultType) in both
WbActions/index.tsx and WbUpload.tsx to replace the inline ternary; ensure the
helper uses the same RecordCounts key type and wbText references so behavior and
localization remain identical.
In `@specifyweb/frontend/js_src/lib/components/WbPlanView/State.tsx`:
- Line 101: The container class string in State.tsx currently uses the Tailwind
forced modifier `!w-1/3` to override dialogClassNames.normalContainer; remove
the `!` override and instead add a dedicated size variant (e.g.,
dialogClassNames.smallContainer or dialogClassNames.normalContainerSmall) in the
dialogClassNames definitions and use that variant here, or adjust
normalContainer to accept size variants so you can replace
`${dialogClassNames.normalContainer} !w-1/3 max-h-[90%]` with a non-forced
combination that references the new variant (keep the rest like max-h-[90%]
unchanged).
In `@specifyweb/frontend/js_src/lib/components/WbToolkit/DevShowPlan.tsx`:
- Around line 85-99: handleFileSelected currently only checks JSON syntax via
JSON.parse in the fileToText promise chain but does not validate the object
shape against the expected UploadPlan schema; update the promise chain in
handleFileSelected to parse the text into an object, perform a schema/shape
check (e.g., verify required fields and types expected by UploadPlan such as
baseTableName and uploadable or use a lightweight validator), and only call
setUploadPlane(text) when the validation passes; on validation failure call
showInvalid (or throw) so the existing catch handles it; reference
handleFileSelected, fileToText, setUploadPlane, UploadPlan and showInvalid when
making the change.
In `@specifyweb/frontend/js_src/lib/localization/wbPlan.ts`:
- Around line 807-826: Remove the inline translations for all locales and leave
only the 'en-us' entries for the two new localization keys so they follow the
project's Weblate workflow: update the invalidJsonFile and
invalidJsonFileDescription objects to contain only the 'en-us' string values
(e.g. 'The selected file is not valid JSON.' and 'Please select a valid JSON
data set mapping file.' respectively), removing the other locale keys (ru-ru,
es-es, fr-fr, uk-ua, de-ch, pt-br, hr-hr) so translators will add them via
Weblate.
In `@specifyweb/frontend/js_src/lib/localization/workbench.ts`:
- Around line 980-989: The new localization object key metadata currently
includes translations for many locales; remove all non-English entries and leave
only the 'en-us' value for metadata so new keys follow the Weblate workflow
(i.e., keep metadata: { 'en-us': 'Metadata' } and remove
'de-ch','es-es','fr-fr','ru-ru','uk-ua','pt-br','hr-hr') so translations are
later added via Weblate.
🪄 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 Plus
Run ID: 2cf96f8c-9e64-4b99-8101-228a215cf666
⛔ Files ignored due to path filters (1)
specifyweb/frontend/js_src/lib/components/Attachments/__tests__/__snapshots__/AttachmentCell.test.tsx.snapis excluded by!**/*.snap
📒 Files selected for processing (22)
specifyweb/frontend/js_src/lib/components/Atoms/Icons.tsxspecifyweb/frontend/js_src/lib/components/Atoms/className.tsspecifyweb/frontend/js_src/lib/components/ExpressSearchConfig/ExpressSearchConfigDialog.tsxspecifyweb/frontend/js_src/lib/components/LocalityUpdate/Status.tsxspecifyweb/frontend/js_src/lib/components/Molecules/SvgIcon.tsxspecifyweb/frontend/js_src/lib/components/Preferences/UserDefinitions.tsxspecifyweb/frontend/js_src/lib/components/WbActions/WbNoUploadPlan.tsxspecifyweb/frontend/js_src/lib/components/WbActions/WbRollback.tsxspecifyweb/frontend/js_src/lib/components/WbActions/WbUpload.tsxspecifyweb/frontend/js_src/lib/components/WbActions/index.tsxspecifyweb/frontend/js_src/lib/components/WbPlanView/Mapper.tsxspecifyweb/frontend/js_src/lib/components/WbPlanView/MapperComponents.tsxspecifyweb/frontend/js_src/lib/components/WbPlanView/README.mdspecifyweb/frontend/js_src/lib/components/WbPlanView/State.tsxspecifyweb/frontend/js_src/lib/components/WbPlanView/uploadPlanParser.tsspecifyweb/frontend/js_src/lib/components/WbToolkit/DevShowPlan.tsxspecifyweb/frontend/js_src/lib/components/WorkBench/DataSetMeta.tsxspecifyweb/frontend/js_src/lib/components/WorkBench/RecordSet.tsxspecifyweb/frontend/js_src/lib/components/WorkBench/Results.tsxspecifyweb/frontend/js_src/lib/localization/batchEdit.tsspecifyweb/frontend/js_src/lib/localization/wbPlan.tsspecifyweb/frontend/js_src/lib/localization/workbench.ts
github-project-automation
Bot
moved this from 📋Back Log
to Dev Attention Needed
in General Tester Board
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #8153
Fixes #8155
Fixes #8014
Fixes #4484
This PR introduces a number of different UI improvements, localization updates, and bug fixes centered around the WorkBench data mapping and upload workflows:
Terminology Updates ("Upload Plan" ➔ "Mapping"):
chooseExistingPlanbecomes "Choose Existing Mapping",uploadPlanis now "Import/Export Mapping").Record Counts & Live Validation:
TableRecordCountscomponent.dataCheck) by default (defaultValue: truein user preferences).Import/Export Mapping Enhancements:
.jsonfile in addition to exporting.Data Mapping Dialog & Usability Improvements:
WorkBench
WorkBench Attachments
UI Fixes:
dy=".35em"on<text>elements inSvgIcon.tsxto fix icon rendering bugs in Firefox.dialogClassNames.extraWideContainerto prevent container shifting and clipping.Checklist
self-explanatory (or properly documented)
specify7/specifyweb/specify/management/commands/run_key_migration_functions.py
Line 50 in ea04665
Testing instructions
1. Live Validation & Upload Record Counts (Fixes #8153)
2. Mapping Import/Export & Clear Confirmation
.jsonfile..jsonfile from your local machine. Verify that uploading a corrupt or invalid JSON file presents an invalid JSON error dialog.3. Base Table Selection Dialog
4. SvgIcon Alignment in Firefox (Fixes #8155)
5. Express Search Dialog Shifting (Fixes #8014)
Summary by CodeRabbit
Release Notes
New Features
Improvements