Repository navigation
Conversation
…Ctrl+S Adds "Working from home" and "On vacation" presets plus a Clear status row to the presence menu, independent of presence/DND (picking Active/Away never clears an existing status, matching real Slack), with a status-bar segment showing what's active. Fixes two related staleness bugs found in review: the per-team cache was losing the custom status on every incoming presence/DND event and on workspace activation, and the status bar wasn't refreshed on workspace switch. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
[Medium impact] The PR appears safe to merge, with no new blocking issues found. SummaryAdds per-workspace WFH, vacation, and clear-status choices to
Reviews (3) · Last reviewed commit: "fix(status): correct truncation budget m..." · Reviewed by Greptile |
|
🤖 Riley Reviewer (AI review for @gammons). Nothing here was run locally; checks come from CI, Greptile and reading the code. Thanks @SieuJS, and welcome! This is your first PR here. The write-up is thorough: it lists what's out of scope and the three staleness bugs you caught yourself. The I/O stays in
Findings Fix before merge:
Optional:
Verdict: changes requested. Finding 1 means the status display and "Clear status" only work for a status set in the current slk session. Fixing 1 and 2 unblocks it. Flagged for @gammons: a user-visible feature over 300 changed lines, and it adds a file under |
Review (Greptile + a maintainer pass) found the first draft cached self status in a second, client-only store written only by Apply(), which: lost the status on restart, never reflected an edit from the real Slack client, had no expiry, and displayed a failed API call as if it had succeeded. Self-status now reads from presenceController.peers (the same store already used for everyone else's custom status, seeded and kept live with no self-filter), fixing all four at once with less code. Apply() no longer writes custom status at all for the three preset actions; the display only updates once the real UserStatusChangeMsg echo arrives. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
Thanks for the thorough review, both — this surfaced a real architecture mistake, not just a bug. Finding 1 (self-status architecture) — fixed. You're right that this should read from the existing peer-status store instead of a second cache. Reworked: Finding 2 (failed request shows as applied) — fixed as a side effect of the above. Since nothing is written optimistically anymore, the status bar/checkmark only ever reflect the real event. A failed Design doc — rewritten to describe the actual shipped architecture and why the first draft was wrong, with the session-specific handoff notes trimmed per your note.
Left as-is (flagging rather than silently skipping):
All new/changed behavior is TDD'd (watched red before green), full suite green under |
|
🤖 Riley Reviewer (AI review for @gammons). Nothing here was run locally; checks come from CI, Greptile and reading the code. Thanks @SieuJS, this is a clean rework.
Findings Fix before merge (both small, and both new with this rework: slk now shows any status set in Slack, not just the two presets):
Optional:
Verdict: needs minor changes. Fixing 1 and 2 unblocks it. Flagged for @gammons: a user-visible feature over 300 changed lines that also edits |
Review (Greptile) found that an emoji-only status (valid in Slack, no status text) was dropped everywhere: mode_normal.go discarded the emoji return value from CustomStatus entirely, so the status bar segment and the menu's Clear-status row both gated visibility on status text alone. SetCurrentCustomStatus/buildItems now take the emoji too, and visibility checks text-or-emoji instead of text-only. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
A long status from the real Slack client (up to 100 chars) could push the status bar over its exact-width budget or hide the connection indicator, now that the self-status redesign shows any status, not just this feature's own two short presets. The custom-status segment now degrades to fit: full text, then emoji + truncated text + ellipsis, then emoji alone, then dropped entirely, before every other segment is measured. Also corrects the design doc's review-history section, which mischaracterized two automated review passes as including a maintainer review. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Local codex-batch-review of the previous commit (not run before it was written) found three real bugs in the status-bar truncation fix: the separator budget double-counted when the segment landed between two other segments instead of at an edge; the truncation loop hand-rolled rune-by-rune width summation instead of reusing this codebase's existing muesli/reflow/truncate.StringWithTail (used in internal/ui/presencemenu and internal/ui/thread/breadcrumb.go, among others) — the hand-rolled version can mismeasure combined grapheme clusters and let a segment exceed its budget; and a budget that fit the emoji but not one character of text produced a bare "emoji …" instead of falling back to emoji alone. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
Both findings from the latest review fixed, pushed at e261bf7: Emoji-only status (Greptile P2 + the "needs minor changes" review). Long status overflowing the status bar. Fair point — the redesign made this reachable (any status from the real Slack client, not just this feature's own two short presets, and Slack allows up to 100 chars). The custom-status segment now degrades to fit: full text → emoji + truncated text + ellipsis → emoji alone → dropped entirely, sized against whatever room is actually left after every other segment. I also ran a local review pass on that fix before pushing and caught three more real issues in it before you'd see them: the truncation budget math double-counted a separator when the segment landed between two other segments instead of at an edge (always needs exactly one more separator than the other segments' joined width already has, not one per side); the truncation loop hand-rolled rune-by-rune width summation instead of reusing this codebase's own Design doc's review-history section updated to record all of this. Full checklist green ( |
|
🤖 Riley Reviewer (AI review for @gammons). Nothing here was run locally; checks come from CI, Greptile and reading the code. Thanks @SieuJS. Both findings are fixed, with tests for each.
Catching the separator double-count yourself before pushing was a good catch.
Findings (at e261bf7), all optional
Verdict: ready to merge. Flagged for @gammons: a user-visible feature over 300 changed lines that also edits |
Summary
Closes #356.
Ctrl+Spresence menu — independent of presence/DND, scoped per-workspace like presence/DND already are.🏠 Working from home) when a status is active; the menu shows a✓checkmark on the matching preset when reopened.status_expiration); vacation has no expiration and is cleared manually.Bugs found and fixed during review (not present in the initial draft)
Two rounds of review (self + two independent read-only passes) surfaced three real staleness bugs in how the cached custom status survives events unrelated to the status itself, all fixed with regression tests:
presenceController.Set()rebuiltworkspaceStatusfrom a zero-value struct on every incomingStatusChangeMsg— which fires on every reconnect and on any presence/DND change — silently wiping the cached custom status even though Slack still had it set server-side.InitialActiveworkspace-activation path: workspaces connect concurrently, and a non-default workspace can finish connecting and be switched to (with a status set on it) before the slower, configured-default workspace'sInitialActivemessage arrives and reclaims the active slot.Explicitly out of scope
status_expiration, butslknever locally re-checks it — unlike peer statuses, which already have apeerStatusTickthat expires them in the UI every minute. Your own status has no deadline field at all today. Concretely: leaveslkrunning past midnight with WFH set, and Slack clears it server-side, but the menu checkmark and status-bar segment keep showing "Working from home" until something else happens to touch the cache. Fixing this would mean adding a deadline field to the self-status cache and a tick loop mirroring the peer one — a reasonable follow-up, not bundled into this PR.Test plan
go build ./...go vet ./...gofmt -l .(empty)golangci-lint run(0 issues)go test ./... -race(full suite green; oneTestWriteMacOSClipboardflake confirmed environmental/pre-existing — passes standalone and on full-suite rerun, zero overlap with this diff)🤖 Generated with Claude Code