refactor: clean up results panel toolbar layout#70
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ 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: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThis PR releases v1.0.15, primarily refactoring the results panel toolbar with a unified jump-to-column searchable dropdown, grouped export dropdown, icon-only row action buttons, discard confirmation dialog, and updated discard behavior to filter local state instead of re-executing queries. ChangesVersion Release v1.0.15
Results Panel Toolbar Refactor
Sequence Diagram(s)sequenceDiagram
participant User
participant Dropdown as Jump Dropdown
participant Grid as DataGrid
User->>Dropdown: Click or type to search
Dropdown->>Dropdown: Filter columns by search
User->>Dropdown: Select column
Dropdown->>Grid: Update gridSelection
Grid->>Grid: Scroll and focus column
sequenceDiagram
participant User
participant DiscardBtn as Discard Button
participant ConfirmDialog
participant MainContent
participant Grid as Grid State
User->>DiscardBtn: Click Discard
DiscardBtn->>ConfirmDialog: Show confirmation
User->>ConfirmDialog: Confirm
ConfirmDialog->>MainContent: onDiscard()
MainContent->>Grid: Filter _isNew rows<br/>Clear _isNew/_isModified flags
Grid->>Grid: Update local state
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
src/components/results/ResultsPanel.tsx (1)
467-545: ⚡ Quick winAdd explicit accessible names to icon-only action buttons.
Buttons at Line 467+ are icon-only and currently depend on
title. Please addaria-labelso screen readers reliably announce actions.🤖 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/components/results/ResultsPanel.tsx` around lines 467 - 545, Several action buttons are icon-only and rely on title text, making them inaccessible to some screen readers; add explicit aria-label attributes to each icon-only button (the Add/Duplicate/Delete/Save/Discard/Export buttons that render Plus, Copy, Trash2, CheckCircle, XCircle, and Download) so they announce their purpose reliably (e.g., aria-label="Add new row", "Duplicate row", "Delete row", "Save changes", "Discard changes", "Export data"), keeping the existing title props and ensuring the conditional Discard button still receives the aria-label when rendered; update the JSX for the buttons inside ResultsPanel (the handlers onAddRow, onDeleteRow, onSave/onRefresh, onDiscard and the export toggle) to include these aria-labels.
🤖 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 `@CHANGELOG.md`:
- Around line 7-11: Add a footer reference link for the new [1.0.15] release and
update the existing [Unreleased] compare base so the compare link is not
pointing at v1.0.11; specifically, in CHANGELOG.md add a matching `[1.0.15]:
<repo_url>/releases/tag/v1.0.15` reference (or the repo-relative compare URL
format your project uses) and change the `[Unreleased]:
.../compare/v1.0.11...HEAD` entry to compare from v1.0.15 (e.g.
`.../compare/v1.0.15...HEAD`) so both the [1.0.15] and [Unreleased] headings
have correct footer links.
In `@src/components/layout/MainContent.tsx`:
- Line 2481: The current setResults call only strips _isModified flags but
leaves edited values intact; update the discard logic in the setResults handler
so that for each row marked _isModified you restore the original values (e.g.,
use a stored snapshot field like _original or look up the row in the unmodified
baseline) and then remove the metadata flags. Specifically, change the mapping
inside setResults(prev => ...) to: if r._isNew drop it, if r._isModified replace
the row with r._original (or the matching baseline row) and remove
_isModified/_isNew/_original fields, otherwise keep the row as-is; reference the
setResults call and the row properties _isNew, _isModified and _original when
implementing this.
In `@src/components/results/ResultsPanel.tsx`:
- Line 528: The confirmation dialog text is inaccurate: update the
confirmDialog.confirm call in ResultsPanel (the invocation where const confirmed
= await confirmDialog.confirm(...)) so the message reflects client-side-only
discard behavior (e.g., "Discard all local changes? This will not affect server
data.") and keep title "Discard Changes" and type "warning"; ensure the new copy
is concise and unambiguous about no server/database reload occurring.
- Around line 437-459: The column jump uses the full columns array but the
rendered grid may use displayColumns, causing wrong targets; change the click
handler to compute and use the active column list (e.g., const activeCols =
displayColumns || columns), filter/search against activeCols with
columnDropdownSearch, derive the column index with activeCols.indexOf(c), and
use that index when building setGridSelection's current.cell/current.range and
when calling gridRef.current?.scrollToColumn(idx); also use activeCols for the
"no matching columns" check so behavior matches the displayed grid.
---
Nitpick comments:
In `@src/components/results/ResultsPanel.tsx`:
- Around line 467-545: Several action buttons are icon-only and rely on title
text, making them inaccessible to some screen readers; add explicit aria-label
attributes to each icon-only button (the
Add/Duplicate/Delete/Save/Discard/Export buttons that render Plus, Copy, Trash2,
CheckCircle, XCircle, and Download) so they announce their purpose reliably
(e.g., aria-label="Add new row", "Duplicate row", "Delete row", "Save changes",
"Discard changes", "Export data"), keeping the existing title props and ensuring
the conditional Discard button still receives the aria-label when rendered;
update the JSX for the buttons inside ResultsPanel (the handlers onAddRow,
onDeleteRow, onSave/onRefresh, onDiscard and the export toggle) to include these
aria-labels.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8b9e6ba3-6f68-4368-b17c-82f0013ea16e
⛔ Files ignored due to path filters (1)
src-tauri/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (6)
CHANGELOG.mdpackage.jsonsrc-tauri/Cargo.tomlsrc-tauri/tauri.conf.jsonsrc/components/layout/MainContent.tsxsrc/components/results/ResultsPanel.tsx
|
Reviewed end-to-end. The UI refactor is solid — the searchable Jump-to-column dropdown, the Export-as menu, the icon-only toolbar, and the Discard confirmation are all real improvements. Bundling the fixes I'm pushing in a moment so v1.0.16 can go out tonight. 🔴 Blocker — Discard does not actually discard modifications
onDiscard={() => {
setResults(prev => prev.filter(r => !r._isNew).map(({ _isNew, _isModified, ...rest }) => rest));
}}This filters out unsaved new rows ✅, but for modified existing rows it only strips the Net effect on the modified-row case: edited value stays in the UI, the row is no longer flagged dirty, Save becomes a no-op ( Fix I'll push — hybrid: re-run the SELECT only if there are modifications; otherwise filter client-side (preserves your perf win for the new-rows-only case, which is the most common): onDiscard={() => {
const hasModifications = results.some(r => r._isModified && !r._isNew);
if (hasModifications && lastSelectQueryRef.current) {
executeQuery(lastSelectQueryRef.current);
} else {
setResults(prev => prev.filter(r => !r._isNew).map(({ _isNew, _isModified, ...rest }) => rest));
}
}}🟡 SAVE button lost the dirty-count badge
<span className="text-[8px] font-bold">
SAVE{results.filter(r => r._isNew || r._isModified).length > 0 ? ` (${results.filter(r => r._isNew || r._isModified).length})` : ""}
</span>The 🟡 Keyboard navigation regression in Jump-to-columnThe old 🟡 Escape doesn't close the Export dropdownInconsistent with the Column dropdown which does. One-line fix; adding to the 🔵 Stale version bumpPR bumps ✅ What's good (no changes from me)
Pushing the fixes in a moment. Thanks for the work, @kix007! |
Four review findings on @kix007's refactor (#70): 1. **Discard now actually discards modifications.** The original `onDiscard` replaced `executeQuery(lastSelectQueryRef.current)` with a client-side filter that stripped `_isNew`/`_isModified` flags from existing rows but did NOT revert the actual edited cell values. Nothing in the codebase preserves originals — cell-edit at ResultsPanel.tsx:243-247 just overwrites with `_isModified: true` — so the only way to revert a modification is a server re-fetch. Hybrid handler: re-runs SELECT when there are real modifications, falls back to the cheap client-side filter when only new (client-only) rows need to be dropped. Preserves the perf win for the common case (user added rows, changed their mind) without leaving a phantom-modified state for the edit case. 2. **Save button regains a dirty-row count.** Going icon-only dropped the "SAVE (3)" inline count, which was the only at-a-glance signal of how many rows are pending. Restored as a small emerald badge in the top-right of the Save button. Caps at "99+". Tooltip also reports the live count. 3. **Keyboard navigation in Jump-to-column.** The replaced `<select>` had browser-default keyboard nav; the new dropdown only handled Escape. Added Arrow up/down to move through the filtered list, Enter to jump, with the active item highlighted via `--color-accent`/15 and `scrollIntoView({ block: "nearest" })` to keep it in view. Mouse hover sets the highlight too, so keyboard and mouse cursors stay in sync. 4. **Escape now closes the Export dropdown.** Was inconsistent with the Column dropdown (only that one handled Escape). Added a document-level keydown listener gated on `showExportDropdown` so the dropdown closes on Esc regardless of where focus is. Note: the stale `1.0.15` release commit from the original branch was dropped during rebase onto main (which already shipped v1.0.15); the CHANGELOG entry will be folded into v1.0.16 at release time.
NicolaasGrobler
force-pushed
the
refactor_data_table_layout
branch
from
1d8f724 to
2fb3c17
Compare
|
Pushed fixes in 2fb3c17. The branch is also rebased onto current What changed vs your original:
Force-push notice: I had to rewrite history (rebase onto main + drop the release commit + add the fix commit). If you had local commits on this branch, run CHANGELOG entry will move to v1.0.16 at release time. Ready to merge from my side — letting Nicolaas drive the merge & release timing. |
Two CodeRabbit findings I missed in the first pass: 1. **Jump-to-column was indexing into the wrong array.** The grid renders with `displayColumns || columns` (line 876), but the dropdown was filtering and indexing against `columns` alone. In multi-result mode those diverge, so clicking a column name jumped to the wrong cell. Now consistently uses `activeColumns = displayColumns || columns` for the list, the filter, the no-matches check, and the index passed to `setGridSelection` + `scrollToColumn`. 2. **Discard confirmation message lied.** The dialog said "reload from database" but the hybrid handler only reloads when there are real modifications; for new-rows-only it's a client-side filter. Changed to "Discard all unsaved local changes? This cannot be undone." — accurate for both code paths.
3 participants
Summary
Changed files
Summary by CodeRabbit
v1.0.15 Release Notes
New Features
Improvements