-
Notifications
You must be signed in to change notification settings - Fork 60
fix: --add-workspace can write a config slk refuses to load - #148
fix: --add-workspace can write a config slk refuses to load #148piotrsynowiec wants to merge 1 commit into
Conversation
Re-running --add-workspace pre-selected every workspace, including the ones already configured. The config writer de-duplicated on the slug, so a second run wrote a second [workspaces.<slug>-2] block carrying the same team_id — and config.Load rejects that, so slk stopped starting until the file was hand-edited. A picker default turned into a broken install. Two fixes, deliberately independent: The writer now skips a team_id it already has. Keying the guard there rather than on the caller remembering to filter makes it hold no matter who calls it, including a future caller that has not been written yet. The picker pre-selects only workspaces that are not configured, and labels the rest. Re-selecting one is still allowed — it refreshes the token — it just no longer happens by accident. configuredTeamIDs decodes the file directly instead of going through config.Load. Load validates, and validation is exactly what fails on a config that already carries a duplicate — routing through it would make the guard useless in the one case it exists for, and a third run would append a third block to a file slk already refuses to start on. Caught by a test that constructs that broken state on purpose.
gammons
commented
Aug 22, 2026
Nice fix — keying the guard on team_id at the writer and decoding the TOML directly (so recovery from an already-broken config works) is the right call, and the tests cover the important states.
One thing before merge, @piotrsynowiec: the new picker strings are in Polish (— już dodany, Wszystkie workspace'y..., Nowe są zaznaczone; spacja przełącza...) while the rest of the UI — including this same dialog's title and the surrounding onboarding flow — is English. Could you switch these to English for consistency?
gammons
commented
Sep 3, 2026
Real bug, correctly diagnosed, and the approach is sound — but there's a blocker.
The bug is confirmed, and it's self-perpetuating. onboarding.go:49-52 pre-selects every workspace, onboarding.go:106 de-dupes on slug only so a second run yields acme-2, appendWorkspaceConfigBlock appends a block with the same team_id, and internal/config/workspaces.go:74-77 then refuses to load it. And because existingSlugs routes through config.Load (add_workspace_config.go:68-71), which now errors and returns an empty set, uniqueSlug hands back the base slug — so a third run writes a duplicate TOML table key, a parse error stacked on top of a validation error. Bricking someone's install from the happy path of the onboarding command is about as bad as first-run UX gets.
Two independent guards (idempotent writer + smarter picker default) is good defensive design, and your justification for bypassing config.Load in configuredTeamIDs is correct — Load fails on precisely the file the guard exists to recover.
Blocker: the PR contains untranslated Polish UI strings.
label += " — już dodany" fmt.Println(dimStyle.Render(" Wszystkie workspace'y z aplikacji desktopowej są już dodane.")) fmt.Println(dimStyle.Render(" Zaznacz któryś, żeby odświeżyć jego token, albo wyjdź (Ctrl+C).")) Description("Nowe są zaznaczone; spacja przełącza, enter zatwierdza.")
The last one replaces the existing English string at onboarding.go:66. slk is English-only with no i18n layer — grep -P '[ąćęłńóśźżĄĆĘŁŃÓŚŹŻ]' across all non-test Go source on main returns zero matches. These need to go back to English, including restoring onboarding.go:66.
Second issue: the guard misses the legacy config format it needs to cover. configuredTeamIDs only reads the team_id field and ignores team-ID-keyed blocks, which config.Load explicitly supports (internal/config/workspaces.go:66-68, isTeamIDKey at workspaces.go:19). Reproduced:
configuredTeamIDs = map[] <- legacy [workspaces.T011B427MLP] invisible
resulting config:
[workspaces.T011B427MLP]
sidebar_width = 30
[workspaces.acme]
team_id = "T011B427MLP"
GUARD MISSED LEGACY KEY -> config.Load now fails:
workspaces "T011B427MLP" and "acme" both reference team_id "T011B427MLP"
So a user on a legacy config hits the exact bug this PR fixes. The picker's already map has the same gap. Two lines: also treat a key matching ^[TE][A-Z0-9]{6,}$ as a configured team ID.
Smaller:
filepath.Join(xdgConfig(), "config.toml")is now built twice inonboarding.go;newCountduplicateslen(chosen).TestConfiguredTeamIDs_ReadsSingleQuotedValuestestsgo-toml, not your code. I'd drop it.TestConfiguredTeamIDs_WorksOnAConfigThatFailsValidationis well chosen — it pins the design decision your description flags. Keep that one.- Nothing covers the
onboarding.gopicker change.addWorkspaceisn't testable as written so I won't insist, but the description shouldn't imply coverage it doesn't have. existingSlugs's own failure mode (empty set on a broken config, leading to a duplicate table key) is still there — now masked by the new guard rather than fixed.
Your CI lint failure is not your fault: old golangci-lint panicking under go1.27, fixed on main by 6d39fe5. Rebase and it clears.
gammons
commented
Sep 3, 2026
#171 has landed and I've re-run CI here — the lint failure is gone and this is green now. That was the stale golangci-lint/go1.27 issue, not anything you did.
Note that #171 also enabled gofmt as an enforced lint check, so please run gofmt -w over your changes when you push the next revision.
My review above still stands — that's what's needed to move this forward.
What breaks
Running
slk --add-workspacea second time can leaveconfig.tomlin a stateslkrefuses to load, so the client stops starting until the file is hand-edited. A picker default turns into a broken install.Why
Two things combine:
--add-workspaceto add one workspace therefore re-added all of them by default.team_id. A second pass wrote a second[workspaces.<slug>-2]block carrying the sameteam_id.config.Loadrejects duplicate team IDs, so the very next launch fails. Worse, the failure is self-perpetuating:--add-workspacewas one of the few things still runnable, and running it again appended a third block to a fileslkalready would not start on.The fix
Two changes, deliberately independent so neither depends on the other holding:
The writer skips a
team_idit already has. Keying the guard at the writer rather than on callers remembering to filter makes it hold no matter who calls it, including callers not yet written.The picker pre-selects only unconfigured workspaces, and labels the rest. Re-selecting one is still allowed — that is how you refresh a token — it just no longer happens by accident.
One implementation note worth flagging
configuredTeamIDsdecodesconfig.tomldirectly instead of going throughconfig.Load.This is deliberate.
Loadvalidates, and validation is exactly what fails on a config that already carries a duplicate. Routing the guard throughLoadwould make it useless in the one case it exists for — recovering a file that is already broken — and a third run would keep appending. There is a test that constructs that broken state on purpose to pin this down.Testing
cmd/slk/add_workspace_config_dedupe_test.gocovers the duplicate-team_idguard, the slug-collision path, and recovery from an already-broken config.Full suite passes.
🤖 Generated with Claude Code