fix(collab): preserve setup step in browser history#3419
Conversation
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
|
|
||
| export function getSetupWorkspace(searchParams: SearchParamReader): WorkspaceSelection | null { | ||
| const organizationId = searchParams.get('organizationId'); | ||
| if (organizationId) return { type: 'org', id: organizationId, name: '' }; |
There was a problem hiding this comment.
WARNING: Org workspace name is silently empty when reconstructed from URL
When navigating back or sharing a URL, getSetupWorkspace restores the org workspace with name: ''. The WorkspaceSelector currently uses org.organizationId (from the fetched list) for display, so it doesn't visibly break today. However, workspace.name could be read elsewhere (e.g. in future display code or passed to APIs) and silently produce an empty string. Consider fetching/resolving the org name from the list once loaded, or at minimum add a comment documenting this invariant.
| ).toBe('/collab?step=1&organizationId=0c06733d-0f2c-42ec-921b-c312fb190427'); | ||
| }); | ||
|
|
||
| test('defaults step two to personal workspace when organization is not set', () => { |
There was a problem hiding this comment.
SUGGESTION: Missing test case for getInitialSetupState with an organizationId in the URL
There's a test for buildSetupPath with org workspace and one for getInitialSetupState with step=1 (personal), but no test for restoring state when organizationId is present in the URL. A round-trip test like the following would catch regressions:
test('restores org workspace state from URL', () => {
const params = new URLSearchParams({ step: '1', organizationId: 'some-id' });
expect(getInitialSetupState(params)).toEqual({
stepIndex: 1,
workspace: { type: 'org', id: 'some-id', name: '' },
});
});| const setupSearch = searchParams.toString(); | ||
| const trpc = useTRPC(); | ||
| const [stepIndex, setStepIndex] = useState(0); | ||
| const initialSetupState = getInitialSetupState(searchParams); |
There was a problem hiding this comment.
SUGGESTION: getInitialSetupState is called unconditionally on every render
getInitialSetupState(searchParams) is called in the render body on every render, but its result is only used to seed useState (lines 42–46). React ignores the initializer value on subsequent renders, so this is a wasted computation on every re-render. Consider using the lazy initializer form of useState or wrapping the call in useMemo if the computation becomes heavier, or restructuring to inline the initial values directly in the useState calls.
Code Review SummaryStatus: 3 Issues Found | Recommendation: Address before merge Executive SummaryOrg workspace name is silently empty when restored from URL, which is a latent data-quality issue in the new setup-path state management. Overview
Issue Details (click to expand)WARNING
SUGGESTION
Files Reviewed (4 files)
Fix these issues in Kilo Cloud Reviewed by claude-4.6-sonnet-20260217 · 617,899 tokens Review guidance: REVIEW.md from base branch |
alex-alecu
left a comment
alex-alecu
left a comment
There was a problem hiding this comment.
LGTM after fixing code reviewer comments
2 participants
Summary
/collabsetup and authorize steps in the URL so browser back/forward returns to the previous step instead of leaving the flow.step=1and noorganizationId.Verification
/collabflow in my dev server.Visual Changes
N/A
Reviewer Notes
/collab?step=1; organization setup step two addsorganizationId.