Aside state persisted in query params

[theosiemensrhodes]

Summary

Aside (detail panel) state is now persisted in the URL via query parameters. Opening an aside updates the URL, and refreshing the page or sharing the link reopens the same aside.

What changed

Controlled PageLayout

PageLayout now supports an optional Radix-style controlled pattern with open / onOpenChange props. When provided, the aside open/close state is driven externally. Pages without asides are unaffected.

Classes page (?classId=)

Clicking a class adds ?classId=xxx to the URL. On load, if the param is present, the aside opens and the correct term is automatically selected.

Schedule page (?shiftId=)

Clicking a shift adds ?shiftId=xxx to the URL. On load, the aside opens and the view navigates to the shift's month so it appears in the list/calendar.

Coverage page (?coverageId=)

Clicking a coverage request adds ?coverageId=xxx to the URL. A new coverage.byId backend route was added so the aside can fetch its data independently (previously it read from the in-memory list). Next/prev navigation updates the URL param accordingly.

Calendar view fix

The calendar view now respects selectedDate on initial render, fixing an issue where switching from list to week view would reset the visible date.

[greptile-apps]

Greptile Summary

This PR persists aside/detail-panel state in the URL via nuqs query parameters across the Classes, Schedule, and Coverage pages, enabling deep-linking and page refresh survival. The core approach — a controlled open/onOpenChange pattern on PageLayout, individual byId queries for fetching detail data, and lifting selected-item IDs into URL state — is well-structured and consistent across all three pages.

Key issues found:

• Security (coverage-router.ts): The new coverage.byId tRPC route does not apply the row-level visibility filters enforced by the list route. Non-admin users (those without shifts: view-all) are normally restricted in the list to open requests and requests they're personally involved in. The byId route bypasses this entirely, allowing any authenticated user with coverage: view to fetch any coverage request by ID — a concern made more acute because URL sharing is the specific feature being introduced.
• Type safety (coverage-aside.tsx): An unsafe type cast as CoverageListItem | undefined may hide server/client type mismatches (e.g., date serialization differences) and should be removed.
• React pattern violation (class-list-view.tsx): A side effect (setQueryTerm) is called inside a setState updater function, which violates React's purity requirement and can cause double mutations in Strict Mode.

Confidence Score: 2/5

• The security issue in coverage.byId bypass requires remediation before merge; the other issues are lower severity but should be fixed.
• The byId endpoint's lack of row-level visibility filtering is a meaningful security gap given that URL sharing is the exact scenario this PR enables. Non-admin users can bypass list visibility rules by directly accessing coverage IDs. The side effect in the class-list-view updater and unsafe type cast in coverage-aside are lower-severity issues but represent real violations of React best practices and type safety.
• src/server/api/routers/coverage-router.ts requires the most attention due to the missing visibility filter on the byId route.

_{Last reviewed commit: 45463e4}

[theosiemensrhodes]

@greptile re-review

[greptile-apps]

byId endpoint bypasses row-level visibility filters

The list endpoint passes viewerUserId and viewerRole to listCoverageRequests, which applies row-level security filtering for non-admin users (those without shifts: ["view-all"]). Non-admins are restricted to seeing only open requests (excluding shifts they're already assigned to) and requests where they are the requester or covering volunteer.

The new byId route fetches any coverage request by ID with no equivalent user-scoped visibility filter — it applies only the top-level coverage: ["view"] permission gate. This means a non-admin user with a ?coverageId= URL (e.g., shared by someone else) can fetch coverage request details they would never see in the list — including resolved requests they have no involvement in.

This is particularly impactful because URL sharing is precisely the feature being shipped. Add the same role-based filter here:

Suggested change

	byId: authorizedProcedure({
	permission: { coverage: ["view"] },
	})
	.input(CoverageRequestIdInput)
	.query(async ({ input, ctx }) => {
	const currentUser = ctx.currentSessionService.requireUser();
	const request = await ctx.coverageService.getCoverageRequestById(
	input.coverageRequestId,
	);
	if (
	hasPermission({
	role: currentUser.role as Role,
	permission: { shifts: ["view-all"] },
	})
	) {
	return getListCoverageRequestWithReason(request);
	}
	return getListCoverageRequestBase(request);
	}),
	byId: authorizedProcedure({
	permission: { coverage: ["view"] },
	})
	.input(CoverageRequestIdInput)
	.query(async ({ input, ctx }) => {
	const currentUser = ctx.currentSessionService.requireUser();
	const request = await ctx.coverageService.getCoverageRequestById(
	input.coverageRequestId,
	);
	const canViewAll = hasPermission({
	role: currentUser.role as Role,
	permission: { shifts: ["view-all"] },
	});
	// For non-admins, enforce the same visibility rules as the list endpoint:
	// only allow access to open requests or requests they're involved in.
	if (!canViewAll) {
	const isInvolved =
	request.requestingVolunteer.id === currentUser.id ||
	request.coveredByVolunteer?.id === currentUser.id;
	const isOpen = request.status === CoverageStatus.open;
	if (!isInvolved && !isOpen) {
	throw new NeuronError("Forbidden", NeuronErrorCodes.FORBIDDEN);
	}
	}
	return canViewAll
	? getListCoverageRequestWithReason(request)
	: getListCoverageRequestBase(request);
	}),

[greptile-apps]

Unsafe type cast may hide server/client type mismatch

data as CoverageListItem | undefined forcefully asserts the tRPC response to CoverageListItem. The server returns a union of getListCoverageRequestBase / getListCoverageRequestWithReason, and CoverageListItem is defined as ListCoverageRequestBase | ListCoverageRequestWithReason. If the inferred tRPC return type doesn't match — for instance, because of a date serialization difference where tRPC returns a string but the model expects a Date — the cast will silence the error and cause a silent runtime failure.

Remove the cast and fix any TypeScript errors surfaced by the raw data type instead, or add a Zod/tRPC validation schema on the client side.

[greptile-apps]

Side effect inside a setState updater function

setQueryTerm(linkedClass.termId) is called inside the setSelectedTermId updater function. React's updater functions are expected to be pure — they must not produce side effects. In React Strict Mode (enabled by default in development), React intentionally invokes updaters twice to surface these violations; this will cause setQueryTerm (and its URL mutation) to fire twice on every effect run.

Perform both state mutations directly instead of nesting one inside the other:

Suggested change

	useEffect(() => {
	if (linkedClass?.termId) {
	setSelectedTermId((prev) => {
	if (prev === linkedClass.termId) return prev;
	setQueryTerm(linkedClass.termId);
	return linkedClass.termId;
	});
	}
	}, [linkedClass?.termId, setQueryTerm]);
	useEffect(() => {
	if (linkedClass?.termId && linkedClass.termId !== selectedTermId) {
	setSelectedTermId(linkedClass.termId);
	void setQueryTerm(linkedClass.termId);
	}
	}, [linkedClass?.termId, selectedTermId, setQueryTerm, setSelectedTermId]);