Repository navigation
Polish transfer UI with hero CTAs, status pill, motion, and compact logs - #7
Conversation
📝 WalkthroughWalkthroughThe GUI now uses bounded, duplicate-collapsing transfer logs. It also updates transfer controls, disclosure indicators, footer status content, responsive layout, and motion styling. ChangesGUI update
Estimated code review effort: 3 (Moderate) | ~20 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
![macroscopeapp[bot]](https://avatars.githubusercontent.com/in/900172?s=60&v=4){kind=small}
| /** Matches a trailing repeat counter added by `appendLogLine` ("line ×3"). */ | ||
| const REPEAT_SUFFIX = /^(.*) ×(\d+)$/s; | ||
|
|
||
| /** |
There was a problem hiding this comment.
🟡 Medium src/logUtils.ts:7
appendLogLine misinterprets a genuine croc line ending in ×N as one of its own collapsed repeats. For example, if the log contains "error ×3" (raw croc output) and the next incoming line is "error", the function treats them as duplicates and rewrites the entry to "error ×4", silently corrupting the original message and inflating the repeat count. This happens because REPEAT_SUFFIX matches any ×N suffix, so there's no way to distinguish a suffix produced by this function from text croc actually emitted. Consider tagging generated suffixes (e.g. a non-printable separator or distinct marker) so real croc output is never mistaken for a collapsed repeat.
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @gui/src/logUtils.ts around line 7:
`appendLogLine` misinterprets a genuine croc line ending in `×N` as one of its own collapsed repeats. For example, if the log contains `"error ×3"` (raw croc output) and the next incoming line is `"error"`, the function treats them as duplicates and rewrites the entry to `"error ×4"`, silently corrupting the original message and inflating the repeat count. This happens because `REPEAT_SUFFIX` matches any `×N` suffix, so there's no way to distinguish a suffix produced by this function from text croc actually emitted. Consider tagging generated suffixes (e.g. a non-printable separator or distinct marker) so real croc output is never mistaken for a collapsed repeat.
| const base = match ? match[1] : last; | ||
| if (base === line) { | ||
| const count = match ? Number.parseInt(match[2], 10) + 1 : 2; | ||
| return [...prev.slice(0, -1), `${line} ×${count}`]; |
There was a problem hiding this comment.
🟡 Medium src/logUtils.ts:25
appendLogLine bypasses the max cap when the new line duplicates the last line. For example, appendLogLine(["a", "a"], "a", 1) returns two entries instead of one. The duplicate-collapse branch returns [...prev.slice(0, -1), ...] without trimming to max, so it can exceed the cap. Consider applying slice(-(max - 1)) (or equivalent) before returning in the duplicate branch.
- return [...prev.slice(0, -1), `${line} ×${count}`];
+ return [...prev.slice(0, -1).slice(-(max - 1)), `${line} ×${count}`];🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @gui/src/logUtils.ts around line 25:
`appendLogLine` bypasses the `max` cap when the new line duplicates the last line. For example, `appendLogLine(["a", "a"], "a", 1)` returns two entries instead of one. The duplicate-collapse branch returns `[...prev.slice(0, -1), ...]` without trimming to `max`, so it can exceed the cap. Consider applying `slice(-(max - 1))` (or equivalent) before returning in the duplicate branch.
![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 `@gui/src/App.css`:
- Around line 507-513: Update the background value in the .status-dot rule to
use the Stylelint-compliant lowercase CSS keyword currentcolor, unless the
project’s established convention requires changing the value-keyword-case
configuration instead.
In `@gui/src/logUtils.ts`:
- Around line 17-28: Update appendLogLine to validate max, return an empty array
when max is zero, and ensure every return path is capped to max entries,
including duplicate compression and oversized prev inputs. Add tests covering
max = 0 and an oversized prev whose last line duplicates the appended line.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 52a8cb4c-80d1-48ff-bb82-c298d1f3e689
📒 Files selected for processing (4)
gui/src/App.cssgui/src/App.tsxgui/src/logUtils.test.tsgui/src/logUtils.ts
| .status-dot { | ||
| width: 0.5rem; | ||
| height: 0.5rem; | ||
| border-radius: 50%; | ||
| background: currentColor; | ||
| flex-shrink: 0; | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Fix the Stylelint keyword-case error.
The configured value-keyword-case rule rejects currentColor. Change it to currentcolor, or update the Stylelint rule if camel-case CSS keywords are the project convention.
- background: currentColor;
+ background: currentcolor;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| .status-dot { | |
| width: 0.5rem; | |
| height: 0.5rem; | |
| border-radius: 50%; | |
| background: currentColor; | |
| flex-shrink: 0; | |
| } | |
| .status-dot { | |
| width: 0.5rem; | |
| height: 0.5rem; | |
| border-radius: 50%; | |
| background: currentcolor; | |
| flex-shrink: 0; | |
| } |
🧰 Tools
🪛 Stylelint (17.14.0)
[error] 511-511: Expected "currentColor" to be "currentcolor" (value-keyword-case)
(value-keyword-case)
🤖 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 `@gui/src/App.css` around lines 507 - 513, Update the background value in the
.status-dot rule to use the Stylelint-compliant lowercase CSS keyword
currentcolor, unless the project’s established convention requires changing the
value-keyword-case configuration instead.
Source: Linters/SAST tools
Summary
UI/UX polish pass on the transfer workflow, following up the v0.1.8 daisyUI redesign:
cta-primary) for Start / Cancel / "Start another transfer"; the Send button now reads "Send N files" when paths are staged.aria-live="polite"), replacing the plain workflow-status line.appendLogLineutil collapses consecutive duplicate croc output lines into a×Ncounter (log capped at 200 lines), with 8 new vitest cases inlogUtils.test.ts.▸rotated open via[aria-expanded]CSS.Type
Test plan
cd gui && npm run buildcd gui && npm run test:rust(39 passed)cd gui && npm test(51 vitest cases, incl. 8 newlogUtilstests)Notes
gui/src/App.tsx,gui/src/App.css, newgui/src/logUtils.ts+ tests); no Rust/backend modifications.maindirectly and was moved here via force-push so it goes through review.Summary by CodeRabbit
New Features
Bug Fixes
Tests
Note
Polish transfer UI with hero CTAs, status pill, motion, and compact logs
prefers-reduced-motion.×Nsuffix via a newlogUtils.tsutility, capped at 200 lines; covered by unit tests inlogUtils.test.ts.aria-expandedinstead of toggled in JS.📊 Macroscope summarized e54c8b6. 2 files reviewed, 0 issues evaluated, 0 issues filtered, 0 comments posted
🗂️ Filtered Issues
No issues evaluated.