Source status indicator: distinguish auth-expired from unreachable - #457
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe ChangesDistinguish Expired from Unreachable connection status
Sequence Diagram(s)Not applicable — the changes are a data mapping refinement (enum split and CSS/class selection) without multi-component sequential interactions. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/hooks/src/source_switch.rs (1)
36-44: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winGuard against stale validation results after source switches.
A slower
src.validate().awaitfrom the previous active source can finish after a later switch and overwritestatus, showing the wrong Expired/Unreachable state for the current source.Suggested fix
- let src = active_source.read().clone(); - if matches!(config.peek().active_source, Source::Local) { + let src = active_source.read().clone(); + let expected_source = config.peek().active_source.clone(); + if matches!(expected_source, Source::Local) { status.set(ConnStatus::Online); return; } status.set(ConnStatus::Connecting); spawn(async move { - status.set(status_for(src.validate().await)); + let next_status = status_for(src.validate().await); + if config.peek().active_source == expected_source { + status.set(next_status); + } });🤖 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 `@crates/hooks/src/source_switch.rs` around lines 36 - 44, The validation in source_switch::spawn can write a stale ConnStatus after the active source changes. Update the logic around active_source.read(), src.validate().await, and status.set so the async result is only applied if it still matches the current source when it completes. Use a source identity/version check inside the spawned task before calling status_for(...) and status.set(...), and ignore outdated validation results from previous switches.
🤖 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 `@crates/components/src/source_switcher.rs`:
- Around line 160-168: The connection status dot is currently rendered only
inside the !collapsed branch in source_switcher.rs, so collapsed mode hides the
status indicator. Update the SourceSwitcher rendering logic around the
ss-stk/ss-stat block to always render the dot for both collapsed and expanded
states, while keeping any text/extra details gated by !collapsed.
---
Outside diff comments:
In `@crates/hooks/src/source_switch.rs`:
- Around line 36-44: The validation in source_switch::spawn can write a stale
ConnStatus after the active source changes. Update the logic around
active_source.read(), src.validate().await, and status.set so the async result
is only applied if it still matches the current source when it completes. Use a
source identity/version check inside the spawned task before calling
status_for(...) and status.set(...), and ignore outdated validation results from
previous switches.
🪄 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 Plus
Run ID: a9f5baad-9f63-4324-9636-3e683bc12f7e
📒 Files selected for processing (3)
crates/components/src/source_switcher.rscrates/hooks/src/source_switch.rscrates/server/src/source.rs
| span { class: "ss-tile", i { class: "{active_icon}" } } | ||
| if !collapsed { | ||
| span { class: "ss-stk", | ||
| span { class: "ss-stat", | ||
| span { class: "ss-stat", title: "{dot_title}", | ||
| span { | ||
| class: if conn() == ConnStatus::Online { "ss-dot ss-on" } else { "ss-dot ss-off" }, | ||
| class: "{dot_class}", | ||
| title: "{dot_title}", | ||
| } | ||
| if conn() == ConnStatus::Connecting { | ||
| if connection_status == ConnStatus::Connecting { |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Render the status dot in collapsed mode too.
The dot is only inside if !collapsed, so the icon-only switcher still hides connection state. This misses the linked issue’s collapsed-switcher acceptance criterion.
Suggested fix
-.ss-mini{width:40px;height:40px;padding:0;justify-content:center;border-radius:11px}
+.ss-mini{width:40px;height:40px;padding:0;justify-content:center;border-radius:11px;position:relative}
+.ss-mini .ss-dot{position:absolute;right:7px;bottom:7px;border:1px solid var(--ss-surface)} onclick: move |_| open.set(!open()),
span { class: "ss-tile", i { class: "{active_icon}" } }
+ if collapsed {
+ span {
+ class: "{dot_class}",
+ title: "{dot_title}",
+ }
+ }
if !collapsed {📝 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.
| span { class: "ss-tile", i { class: "{active_icon}" } } | |
| if !collapsed { | |
| span { class: "ss-stk", | |
| span { class: "ss-stat", | |
| span { class: "ss-stat", title: "{dot_title}", | |
| span { | |
| class: if conn() == ConnStatus::Online { "ss-dot ss-on" } else { "ss-dot ss-off" }, | |
| class: "{dot_class}", | |
| title: "{dot_title}", | |
| } | |
| if conn() == ConnStatus::Connecting { | |
| if connection_status == ConnStatus::Connecting { | |
| span { class: "ss-tile", i { class: "{active_icon}" } } | |
| if collapsed { | |
| span { | |
| class: "{dot_class}", | |
| title: "{dot_title}", | |
| } | |
| } | |
| if !collapsed { | |
| span { class: "ss-stk", | |
| span { class: "ss-stat", title: "{dot_title}", | |
| span { | |
| class: "{dot_class}", | |
| title: "{dot_title}", | |
| } | |
| if connection_status == ConnStatus::Connecting { |
🤖 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 `@crates/components/src/source_switcher.rs` around lines 160 - 168, The
connection status dot is currently rendered only inside the !collapsed branch in
source_switcher.rs, so collapsed mode hides the status indicator. Update the
SourceSwitcher rendering logic around the ss-stk/ss-stat block to always render
the dot for both collapsed and expanded states, while keeping any text/extra
details gated by !collapsed.
|
can you fix the linting and conflicts? |
ac5e8e6 to
92244d6
Compare
|
Rebased onto master. In the meantime upstream landed the server side of this independently — |
2 participants
Summary
The source switcher now distinguishes an expired/invalid session from an unreachable server, so the status indicator reports the two states separately instead of collapsing both into a single failure.
Why this matters
Previously a source that failed validation showed one generic failure state, so a user could not tell whether their saved session had expired (re-sign-in needed) or the server was actually unreachable (network problem). Splitting the states lets the UI guide the user to the right remedy. Reported in #427.
Testing
Added a hook test asserting the status mapping resolves to the expired/needs-sign-in state versus the unreachable state for the corresponding validation outcomes.
Closes #427