fix: keep todo dock collapsed while todos exist#297
Conversation
|
Warning Rate limit exceeded
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ 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: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (7)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Code Review
This pull request simplifies the session composer's dock state management by removing the 'closing' and 'clear' states, ensuring the dock remains open as long as todos exist. The todoState logic is streamlined to return only 'hide' or 'open' based on the task count, and the dock's default state is now set to collapsed. Feedback was provided to remove unused hardcoded parameters when calling todoState to clean up the implementation.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
Astro-Han
left a comment
There was a problem hiding this comment.
Review Summary
The change correctly simplifies the todo dock lifecycle, but several issues remain — ranging from dead code to potential product behavior regressions.
P0 — Must Fix
todoStateis a trivial identity function (count > 0 ? "open" : "hide"). It provides zero abstraction value and adds indirection. Inline it into the effect or delete it entirely.
P1 — Should Fix
test.liveis orphaned dead code.ComposerDriverState.liveis no longer consumed anywhere afterlivememo removal. Clean uptesting/session-composer.tsto avoid misleading future test authors.- No mechanism to clear stale todos. The
clear()function that wiped orphaned todos is gone. If a model leaves todos behind, they persist forever. Confirm this is intentional and document it in code comments if so.
P2 — Consider Fixing
todoStateexport still exists with a single test consumer. After inlining, the unit test can assert the trivial logic directly. Remove the export to reduce surface area.- Collapsed state is not remembered.
collapsed: trueresets on every mount. Consider persisting user preference (e.g.,localStorage) so expanding the dock isn't lost on navigation. expectClosed()may be dead code in E2E. The test now usesexpectCollapsed()at the end. VerifyexpectClosed()is still used elsewhere; if not, remove it.
P3 — Nit
- Commit history could be squashed. The second commit ("simplify todo dock state input") is a fixup of the first. Per
AGENTS.md, this should be afixup!commit autosquashed before merge.
2d4e16f to
6babbd7
Compare
Summary
Keep the session Todo dock mounted whenever the current session still has todo data, and make the dock start collapsed by default.
Why
The Todo dock was tied to the session live state, so it disappeared as soon as the turn ended. It also opened by default, which consumed space above the composer. The new behavior keeps existing todos available after the turn ends while showing only the compact collapsed row unless the user expands it.
Related Issue
No issue. The behavior was narrow and confirmed directly in code before implementation.
How To Verify
Screenshots or Recordings
Not attached. The focused E2E covers the visible collapsed, expanded, and post-turn states through the Todo dock probe.
Checklist
dev, and my PR title and commit messages use Conventional Commits in English