Add attachments to existing row in workbench#7068
Open
alesan99 wants to merge 253 commits into
Open
Conversation
Triggered by b8d1889 on branch refs/heads/issue-6075
Triggered by bf9a7eb on branch refs/heads/issue-6075
Triggered by fd2c42c on branch refs/heads/issue-6075
Triggered by d93a3ac on branch refs/heads/issue-6075
Triggered by af95077 on branch refs/heads/issue-6075
Triggered by e7f5281 on branch refs/heads/issue-6075
Triggered by 7af5f3e on branch refs/heads/issue-6075
Triggered by 30ff2dc on branch refs/heads/issue-6075
Use promise chaining for attachment uploading
Contributor
|
@alesan99 could you:
|
Contributor
Author
|
📝 WalkthroughWalkthroughAdds end-to-end attachment support to the workbench: shared upload/delete helpers, an extracted save helper, expanded attachments preview with upload/progress and per-row deletion, viewer slider navigation, wiring to WbView, and row-deletion safety checks. Minor test and README formatting adjusted. ChangesWorkbench Attachments V2
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 2❌ Failed checks (2 inconclusive)
✅ 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 |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Triggered by 535c58a on branch refs/heads/issue-6078
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Triggered by 3c508ce on branch refs/heads/issue-6078
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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/WbSave.tsx`:
- Around line 15-40: handleWorkbenchSave returns immediately because the
ping(...) promise is neither awaited nor returned; change handleWorkbenchSave to
await or return the ping call so callers wait for the PUT to finish.
Specifically, update the body of handleWorkbenchSave to either "return await
ping(...)" or "return ping(...)" (keeping the existing then(...) chain that
calls checkDeletedFail, handleSpreadsheetUpToDate, sets
workbench.cells.cellMeta, calls workbench.utils?.searchCells and
workbench.hot?.render) so the async function properly resolves only after the
network save completes.
In `@specifyweb/frontend/js_src/lib/components/WorkBench/attachmentHelpers.tsx`:
- Around line 248-253: The current flow calls hot.setDataAtCell(row,
attachmentColumn, data) before awaiting the DELETE request, which can leave the
UI out-of-sync if the ping(`/api/specify/spdatasetattachment/${idToDelete}/`, {
method: 'DELETE' }) fails; change the order so you await the DELETE (using ping
and checking its success/response) first and only call hot.setDataAtCell(...)
after a successful deletion (handle and surface errors from ping so you do not
update the cell on failure), referencing the existing attachmentsToCell,
hot.setDataAtCell, ping, and idToDelete symbols when locating the code to
change.
In `@specifyweb/frontend/js_src/lib/components/WorkBench/hooks.ts`:
- Around line 515-521: The delete-guard currently reads renderer metadata
(cellMeta.formattedValue) which is unreliable; instead, for each entry in
removedRows use the dataset model on workbench.dataset to locate the
corresponding row object and inspect its attachment field/link list. Use
getVisualAttachmentsColumn(workbench.dataset, workbench.hot) to identify the
attachments column, map that column to the dataset's column key (or index) and
then check the dataset row's attachment value (e.g., non-empty array or
non-empty link string) to decide whether to return false; remove the dependence
on workbench.hot.getCellMeta(...) and cellMeta.formattedValue.
In
`@specifyweb/frontend/js_src/lib/components/WorkBench/WbAttachmentsPreview.tsx`:
- Around line 303-307: The delete button is incorrectly disabled for every
attachment except the last because the disabled check compares dataSetAttachment
to attachments.at(-1)?.spDataSetAttachment; update the guard so each
attachment's delete button is enabled when it corresponds to the current
dataSetAttachment (or meets the intended permission/ownership rule) by comparing
against the specific attachment instance in the mapping (use the local
attachment variable used when rendering Button.Small rather than
attachments.at(-1)), and ensure the remove handler (the delete flow that updates
ordinals/indexes) reindexes remaining attachments after removal (adjust
whichever function performs deletion to recompute ordinals and update attachment
list state).
🪄 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: f98d5b4c-e0fa-4aa0-93f8-589709d3dc11
📒 Files selected for processing (10)
specifyweb/frontend/js_src/lib/components/DataModel/__tests__/businessRules.test.tsspecifyweb/frontend/js_src/lib/components/PickLists/__tests__/fetch.test.tsspecifyweb/frontend/js_src/lib/components/WbActions/WbSave.tsxspecifyweb/frontend/js_src/lib/components/WbAttachmentViewer/index.tsxspecifyweb/frontend/js_src/lib/components/WbImportAttachments/index.tsxspecifyweb/frontend/js_src/lib/components/WorkBench/WbAttachmentsPreview.tsxspecifyweb/frontend/js_src/lib/components/WorkBench/WbView.tsxspecifyweb/frontend/js_src/lib/components/WorkBench/attachmentHelpers.tsxspecifyweb/frontend/js_src/lib/components/WorkBench/hooks.tsspecifyweb/frontend/js_src/lib/localization/README.md
💤 Files with no reviewable changes (1)
- specifyweb/frontend/js_src/lib/localization/README.md
github-project-automation
Bot
moved this from 📋Back Log
to Dev Attention Needed
in General Tester Board
Fix dataset save not being awaited
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
specifyweb/frontend/js_src/lib/components/WbActions/WbSave.tsx (1)
59-71:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winWrap in
try/finallyto ensure progress bar closes on error.If
handleWorkbenchSavethrows,closeProgressBar()won't execute and the dialog remains visible indefinitely.Suggested fix
const handleSave = async (): Promise<void> => { // Show saving progress bar openProgressBar(); - - await handleWorkbenchSave( - workbench, - searchRef, - checkDeletedFail, - handleSpreadsheetUpToDate, - ); - - closeProgressBar(); + try { + await handleWorkbenchSave( + workbench, + searchRef, + checkDeletedFail, + handleSpreadsheetUpToDate, + ); + } finally { + closeProgressBar(); + } }🤖 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/WbSave.tsx` around lines 59 - 71, The progress bar open/close should be made exception-safe: in handleSave wrap the call to handleWorkbenchSave(...) with a try/finally so openProgressBar() is always paired with closeProgressBar(); call openProgressBar() before the try, await handleWorkbenchSave(workbench, searchRef, checkDeletedFail, handleSpreadsheetUpToDate) inside the try, and call closeProgressBar() in the finally block (rethrow or let errors propagate after finally).
🤖 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.
Outside diff comments:
In `@specifyweb/frontend/js_src/lib/components/WbActions/WbSave.tsx`:
- Around line 59-71: The progress bar open/close should be made exception-safe:
in handleSave wrap the call to handleWorkbenchSave(...) with a try/finally so
openProgressBar() is always paired with closeProgressBar(); call
openProgressBar() before the try, await handleWorkbenchSave(workbench,
searchRef, checkDeletedFail, handleSpreadsheetUpToDate) inside the try, and call
closeProgressBar() in the finally block (rethrow or let errors propagate after
finally).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: a10a8442-067e-408b-b4b5-d99a3da01432
📒 Files selected for processing (2)
specifyweb/frontend/js_src/lib/components/WbActions/WbSave.tsxspecifyweb/frontend/js_src/lib/components/WorkBench/attachmentHelpers.tsx
Member
|
@alesan99 Are all the TODOs finished? |
Contributor
Author
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
specifyweb/frontend/js_src/lib/components/WbActions/WbSave.tsx (1)
59-71:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winProgress bar can get stuck open if save fails.
closeProgressBar()runs only on the success path. IfhandleWorkbenchSaverejects (network error, unexpected status, etc.), the "Saving…" dialog will remain open until the user dismisses it manually. Wrapping the await intry/finallykeeps the UI in a sane state on errors.🛡️ Suggested fix
const handleSave = async (): Promise<void> => { // Show saving progress bar openProgressBar(); - - await handleWorkbenchSave( - workbench, - searchRef, - checkDeletedFail, - handleSpreadsheetUpToDate, - ); - - closeProgressBar(); - } + try { + await handleWorkbenchSave( + workbench, + searchRef, + checkDeletedFail, + handleSpreadsheetUpToDate, + ); + } finally { + closeProgressBar(); + } + };🤖 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/WbSave.tsx` around lines 59 - 71, handleSave currently calls openProgressBar(), awaits handleWorkbenchSave(...), then calls closeProgressBar(), so if handleWorkbenchSave throws the progress bar stays open; wrap the await in a try/finally inside handleSave so closeProgressBar() is always called (and rethrow or let the error propagate after finally), keeping openProgressBar()/closeProgressBar() paired even on network or unexpected failures in handleWorkbenchSave.
🧹 Nitpick comments (1)
specifyweb/frontend/js_src/lib/components/WbActions/WbSave.tsx (1)
26-40: 💤 Low valueConsider using
async/awaitconsistently insidehandleWorkbenchSave.The helper is declared
asyncbut uses a.then()chain. Converting to straightawaitwould make the control flow easier to read and would let future error handling/logging hook into a single try block.♻️ Suggested refactor
- // Send data - return ping(`/api/workbench/rows/${workbench.dataset.id}/`, { - method: 'PUT', - body: workbench.data, - expectedErrors: [Http.NO_CONTENT, Http.NOT_FOUND], - }) - .then((status) => checkDeletedFail(status)) - .then(() => { - handleSpreadsheetUpToDate(); - workbench.cells.cellMeta = []; - workbench.utils?.searchCells( - { key: 'SettingsChange' }, - searchRef.current - ); - workbench.hot?.render(); - }); + // Send data + const status = await ping(`/api/workbench/rows/${workbench.dataset.id}/`, { + method: 'PUT', + body: workbench.data, + expectedErrors: [Http.NO_CONTENT, Http.NOT_FOUND], + }); + checkDeletedFail(status); + handleSpreadsheetUpToDate(); + workbench.cells.cellMeta = []; + workbench.utils?.searchCells({ key: 'SettingsChange' }, searchRef.current); + workbench.hot?.render();🤖 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/WbSave.tsx` around lines 26 - 40, The function handleWorkbenchSave currently returns ping(...).then(...).then(...)—refactor it to use async/await: await the ping call to /api/workbench/rows/${workbench.dataset.id}/ with method 'PUT' and expectedErrors, then call checkDeletedFail(status) (await if it returns a promise), and finally run the post-save steps (handleSpreadsheetUpToDate(), reset workbench.cells.cellMeta = [], call workbench.utils?.searchCells(...searchRef.current) and workbench.hot?.render()) inside the same try block; wrap the await flow in a try/catch so errors can be handled/logged uniformly. Ensure you update references to ping, checkDeletedFail, handleSpreadsheetUpToDate, workbench.cells.cellMeta, workbench.utils?.searchCells and workbench.hot?.render when replacing the .then chain.
🤖 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.
Outside diff comments:
In `@specifyweb/frontend/js_src/lib/components/WbActions/WbSave.tsx`:
- Around line 59-71: handleSave currently calls openProgressBar(), awaits
handleWorkbenchSave(...), then calls closeProgressBar(), so if
handleWorkbenchSave throws the progress bar stays open; wrap the await in a
try/finally inside handleSave so closeProgressBar() is always called (and
rethrow or let the error propagate after finally), keeping
openProgressBar()/closeProgressBar() paired even on network or unexpected
failures in handleWorkbenchSave.
---
Nitpick comments:
In `@specifyweb/frontend/js_src/lib/components/WbActions/WbSave.tsx`:
- Around line 26-40: The function handleWorkbenchSave currently returns
ping(...).then(...).then(...)—refactor it to use async/await: await the ping
call to /api/workbench/rows/${workbench.dataset.id}/ with method 'PUT' and
expectedErrors, then call checkDeletedFail(status) (await if it returns a
promise), and finally run the post-save steps (handleSpreadsheetUpToDate(),
reset workbench.cells.cellMeta = [], call
workbench.utils?.searchCells(...searchRef.current) and workbench.hot?.render())
inside the same try block; wrap the await flow in a try/catch so errors can be
handled/logged uniformly. Ensure you update references to ping,
checkDeletedFail, handleSpreadsheetUpToDate, workbench.cells.cellMeta,
workbench.utils?.searchCells and workbench.hot?.render when replacing the .then
chain.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 426e78a8-751e-4f09-a7c6-f2a7b8e8d3c3
📒 Files selected for processing (1)
specifyweb/frontend/js_src/lib/components/WbActions/WbSave.tsx
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
4 participants
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 #6078
Adds the ability to upload additional attachments to a workbench dataset with attachments. You can also delete attachments that belong to a row.
Not implemented (future updates):
Checklist
self-explanatory (or properly documented)
Testing instructions
main.Summary by CodeRabbit
New Features
Improvements
Documentation