Skip to content

fix: make PR mode the default workflow#17

Merged
julianknutsen merged 2 commits intogastownhall:mainfrom
KalleBylin:fix/default-workflow
Mar 8, 2026
Merged

fix: make PR mode the default workflow#17
julianknutsen merged 2 commits intogastownhall:mainfrom
KalleBylin:fix/default-workflow

Conversation

@KalleBylin
Copy link
Copy Markdown
Contributor

@KalleBylin KalleBylin commented Mar 6, 2026

Closes #16

Why

  • the repo was inconsistent about the default workflow mode: docs and the remote join path treated pr as the default, while unset local config still resolved to wild-west
  • after rebasing onto newer upstream changes, the new web test suite exposed a stale keyboard-selection bug in BrowseList (j followed immediately by Enter could still read the old selection)

What changed

  • default unset config mode to pr
  • persist pr in newly created local configs so local and remote join agree
  • update CLI/help and integration tests so wild-west behavior is explicit where those tests depend on mainline mutations
  • stabilize BrowseList keyboard navigation by keeping the latest selected index available synchronously before navigation (this was failing after updating branch)

Testing

  • go test ./...
  • go vet ./...
  • go test -tags=integration ./internal/remote/
  • go test -tags=integration ./test/integration/offline -run 'TestClaimWanted|TestClaimAlreadyClaimed|TestDoneFullLifecycle|TestStatusFullLifecycle|TestWildWestModeUnchanged|TestConfigSetGetMode'
  • npm --prefix web run check

Out of Scope

wl join --direct is unchanged in this PR. It still affects the join path, but does not implicitly switch the saved workflow mode to wild-west.

Copy link
Copy Markdown
Contributor

@DreadPirateRobertz DreadPirateRobertz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM — approving.

Thorough, well-scoped change that resolves a real inconsistency (issue #16). The default mode now consistently resolves to pr across all code paths: ResolveMode(), Join(), Create(), CLI help text, and tests.

Strengths:

  • Correct approach: default changed in ResolveMode() and explicitly persisted in Join()/Create() config structs, so both new and existing installs behave consistently.
  • Test coverage is strong: new TestResolveMode table-driven test, updated assertions in TestJoin_Success/TestCreate_Success, and lifecycle tests now explicitly opt into wild-west where needed via joinedLifecycleEnv().
  • setMode helper correctly moved from pr_mode_test.go to offline_test.go where it's shared — no duplication.
  • Unused imports (os, filepath) cleaned up from pr_mode_test.go.

CONTRIBUTING.md compliance:

  • Feature branch: fix/default-workflow — correct fix/* convention.
  • Commit message uses present tense ("make PR mode the default").
  • PR body references the issue it closes.

One minor observation (non-blocking): The ResolveMode() function now returns ModePR for both empty string and ModePR input, but returns c.Mode verbatim for anything else (including ModeWildWest). This means an invalid mode string like "typo" would be returned as-is rather than falling back to default. Existing behavior, not introduced by this PR, so not a blocker.

Clean work.

Copy link
Copy Markdown
Contributor

@DreadPirateRobertz DreadPirateRobertz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Clean implementation. Default to PR mode makes sense for Phase 2 — wild-west was the bootstrap phase. Test coverage is solid with the table-driven TestResolveMode and updated lifecycle helpers. LGTM.

KalleBylin and others added 2 commits March 8, 2026 06:18
- Settings.tsx: normalize empty mode to "pr" instead of "wild-west" so
  legacy configs don't silently revert the new default on save
- BrowseList.tsx: reset selection on background poll to prevent stale
  index navigation after items change
- cmd_serve.go: set anonymous hosted client mode to ModePR for
  consistent API contract

Co-Authored-By: Claude Opus 4.6 <[email protected]>
@julianknutsen julianknutsen force-pushed the fix/default-workflow branch from e01e9f6 to 9801ebb Compare March 8, 2026 05:19
@julianknutsen
Copy link
Copy Markdown
Collaborator

Adoption Review

Thanks for the contribution, @KalleBylin! This PR correctly identifies and fixes a real inconsistency — docs and the remote join path treated pr as the default workflow mode, but unset local config still resolved to wild-west. The approach is clean: update ResolveMode(), persist the explicit value in Join/Create, and make integration tests mode-explicit so they don't break when the default changes. The BrowseList keyboard fix is a textbook React stale-closure resolution.

This PR was adopted with maintainer fixes pushed directly to the contributor's branch.

Initial Review

The initial dual-model review (Claude + Codex) found one major issue: Settings.tsx still coerced empty mode to wild-west, meaning opening and saving settings on a legacy workspace would silently revert the new default. Two minor issues were also flagged: the anonymous hosted client in cmd_serve.go serialized mode: "", and the BrowseList background poll didn't reset selection after item refresh.

Maintainer Changes

  • Settings.tsx: normalize empty mode to "pr" instead of "wild-west" in both initial state and load fallback
  • BrowseList.tsx: reset selection on background poll to prevent stale index navigation
  • cmd_serve.go: set anonymous hosted client mode to ModePR for consistent API contract

Final Review Status

All clear — no remaining blockers or majors. The Codex-flagged TUI finding was independently refuted (TUI already uses cfg.ResolveMode()). One minor pre-existing issue noted: background poll resets selection unconditionally even when items haven't changed. Recommended as follow-up, not blocking.

Review Iterations

2 review passes performed.


Adopted via /adopt-pr workflow. Contributor commits preserved.

@julianknutsen julianknutsen merged commit 4c06b63 into gastownhall:main Mar 8, 2026
5 checks passed
@KalleBylin
Copy link
Copy Markdown
Contributor Author

Thank you both for the detailed PRs, very much appreciated!

I have opened up follow up PRs:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Clarify the default workflow mode (PR vs wild-west) and make docs/code agree

3 participants