feat: highlight new pairings, dashboard sorted by filename - #1377
feat: highlight new pairings, dashboard sorted by filename#1377giannatan wants to merge 4 commits into
Conversation
📝 WalkthroughWalkthroughThis change adds dashboard highlighting for newly uploaded documents. Upload results now include entry IDs and names. The upload flow prevents overlapping batches, records successful IDs before refresh, and resets pending state after completion. Dashboard rendering sorts the current folder and applies animated highlighting to matching tiles. ChangesDashboard upload highlight flow
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant UploadArea
participant UploadTools
participant Dashboard
UploadArea->>UploadTools: handleUploadAllDocuments(currentFolder)
UploadTools-->>UploadArea: fulfilled values with { id, name }
UploadArea->>Dashboard: markNewlyUploaded(successfulFiles.map(id))
UploadArea->>Dashboard: updateDashboard()
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
deployment/server/css/dashboard.cssParsing error: Declaration expected. 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 |
zih-syuan
left a comment
zih-syuan
left a comment
There was a problem hiding this comment.
Please rebase this branch onto the latest develop. Thank you!!
| if (successfulFiles.length > 0) { | ||
| const infoBadge = document.getElementById('info-badge'); | ||
| infoBadge.textContent = `Uploaded files: ${successfulFiles.join( | ||
| infoBadge.textContent = `Uplaoded files: ${successfulFiles.join( |
There was a problem hiding this comment.
Is this a typo? Should be Uploaded files
| .map((result) => result.value); | ||
|
|
||
| setTimeout(async () => { | ||
| if (successfulFiles.length > 0) { |
There was a problem hiding this comment.
If batch A succeeds and the next batch has no successful uploads, this guard skips markNewlyUploaded(), so batch A remains highlighted. Could we call it unconditionally so an empty result clears the previous highlight?
| container.classList.add('document-entry'); | ||
| container.setAttribute('draggable', 'true'); // make file or folder draggable | ||
|
|
||
| if (newlyUploadedNames.has(entry.name)) { |
There was a problem hiding this comment.
Different folders can contain entries with the same filename. Since this set persists across folder navigation and createTile() checks only entry.name, an entry in another folder could be highlighted incorrectly. Could this be scoped to the upload folder or track entry IDs?
| /cypress/downloads | ||
|
|
||
| .idea | ||
| .nvmrc |
There was a problem hiding this comment.
Is ignoring .nvmrc related to this feature? If it is only a local file, could this unrelated change be removed from the PR?
…ard is sorted in alphabetical order
3b71837 to
2db961d
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@src/Dashboard/UploadArea.ts`:
- Around line 24-26: Update the upload flow around handleUploadAllDocuments and
the delayed markNewlyUploaded callback so overlapping batches cannot replace
highlights from a newer batch. Either disable the upload action until the
pending batch and its callback complete, or track a monotonically increasing
batch token and ignore callbacks from older batches while preserving the current
updateDashboard behavior.
- Around line 20-22: Update the successfulFiles filtering in
handleUploadAllDocuments() to use an explicit type predicate for fulfilled
results, narrowing the result value to a non-null, non-undefined upload value
before mapping. Preserve the existing exclusion of rejected results and empty
optional values.
🪄 Autofix
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: 8d4c7d82-c7c0-4e39-ba2a-10513496f661
📒 Files selected for processing (5)
deployment/server/css/dashboard.csssrc/Dashboard/Dashboard.tssrc/Dashboard/FileSystem/FileSystemTools.tssrc/Dashboard/UploadArea.tssrc/Dashboard/UploadTools.ts
| modalWindow: ModalWindow, | ||
| currentFolder: IFolder, | ||
| ) { | ||
| if (isUploadPending) { |
There was a problem hiding this comment.
Could we reset isUploadPending in a finally block in both paths? If updateDashboard() throws, the current reset line will not be reached, so all subsequent upload attempts will remain blocked until the page is reloaded.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@src/Dashboard/UploadArea.ts`:
- Around line 49-79: Update both delayed async callbacks in the upload success
and error paths to handle updateDashboard() rejections within the setTimeout
callback. Move spinner removal and modalWindow.hideModalWindow() into each
callback’s finally block, alongside resetting isUploadPending, so cleanup always
runs; preserve the existing success-only info-badge behavior.
🪄 Autofix
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: 2a479e0c-0188-48a1-b8b7-1a75e0fa41d4
📒 Files selected for processing (3)
deployment/server/css/dashboard.csssrc/Dashboard/UploadArea.tssrc/Dashboard/UploadTools.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- deployment/server/css/dashboard.css
- src/Dashboard/UploadTools.ts
| setTimeout(async () => { | ||
| try { | ||
| markNewlyUploaded(successfulFiles.map((file) => file.id)); | ||
| await updateDashboard(); | ||
| spinner.classList.remove('visible'); | ||
| modalWindow.hideModalWindow(); | ||
|
|
||
| if (successfulFiles.length > 0) { | ||
| const infoBadge = document.getElementById('info-badge'); | ||
| infoBadge.textContent = `Uploaded files: ${successfulFiles.join( | ||
| ', ', | ||
| )}`; | ||
| infoBadge.style.display = 'block'; | ||
| infoBadge.style.background = '#9DB2BF'; | ||
| if (successfulFiles.length > 0) { | ||
| const infoBadge = document.getElementById('info-badge'); | ||
| infoBadge.textContent = `Uploaded files: ${successfulFiles | ||
| .map((file) => file.name) | ||
| .join(', ')}`; | ||
| infoBadge.style.display = 'block'; | ||
| infoBadge.style.background = '#9DB2BF'; | ||
| } | ||
| } finally { | ||
| isUploadPending = false; | ||
| } | ||
| }, 2000); | ||
| }) | ||
| .catch((error) => { | ||
| console.log('One or more uploads rejected: ', error); | ||
| setTimeout(async () => { | ||
| await updateDashboard(); | ||
| spinner.classList.remove('visible'); | ||
| modalWindow.hideModalWindow(); | ||
| try { | ||
| await updateDashboard(); | ||
| spinner.classList.remove('visible'); | ||
| modalWindow.hideModalWindow(); | ||
| } finally { | ||
| isUploadPending = false; | ||
| } | ||
| }, 2000); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Handle failures inside the delayed callbacks.
Line 49 and Line 71 create async callbacks that setTimeout() does not await. If updateDashboard() rejects, the outer .catch() cannot handle that rejection. The spinner and modal can remain visible because their cleanup lines do not run.
Catch errors inside each delayed callback. Move spinner and modal cleanup into finally.
Proposed fix
-setTimeout(async () => {
+setTimeout(() => {
+ void (async () => {
try {
markNewlyUploaded(successfulFiles.map((file) => file.id));
await updateDashboard();
- spinner.classList.remove('visible');
- modalWindow.hideModalWindow();
-
if (successfulFiles.length > 0) {
// ...
}
+ } catch (error) {
+ console.error('Could not refresh dashboard after upload:', error);
} finally {
+ spinner.classList.remove('visible');
+ modalWindow.hideModalWindow();
isUploadPending = false;
}
-}, 2000);
+ })();
+}, 2000);Apply the same pattern to the error-path callback.
🤖 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 `@src/Dashboard/UploadArea.ts` around lines 49 - 79, Update both delayed async
callbacks in the upload success and error paths to handle updateDashboard()
rejections within the setTimeout callback. Move spinner removal and
modalWindow.hideModalWindow() into each callback’s finally block, alongside
resetting isUploadPending, so cleanup always runs; preserve the existing
success-only info-badge behavior.
2 participants
ref: issue #1316
Summary by CodeRabbit
New Features
Improvements