Add server side authorization for Gerrit#545
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
📝 WalkthroughWalkthroughAdds server-side Gerrit feature-flag enforcement to a Gerrit route, defaults the client Gerrit flag to enabled when undefined, moves PostHog initialization earlier and boots PostHog with a Gerrit flag set to false, plus minor formatting/import tweaks and a changeset. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client (Gerrit UI)
participant Server as Server (route action)
participant FF as FeatureFlag Service
participant PH as PostHog
Client->>Server: POST /.../gerrit (form/action)
Server->>FF: check featureFlag("gerrit", principal_id)
alt flag == disabled
FF-->>Server: disabled
Server-->>Client: 403 Forbidden
else flag == enabled or undefined
FF-->>Server: enabled
Server->>PH: init PostHog (bootstrap featureFlags.gerrit=false)
Server->>Server: validate form & process intent
Server->>PH: capture $ai_input / $pageview
Server-->>Client: 200 OK (result)
end
Estimated Code Review Effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly Related PRs
Suggested Labels
Suggested Reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@fdm-app/app/root.tsx`:
- Around line 98-102: The bootstrap is explicitly setting featureFlags.gerrit to
false which causes useFeatureFlagEnabled("gerrit") to return false instead of
undefined; remove the "gerrit: false" entry from the bootstrap.featureFlags
object (or omit the gerrit key entirely) so useFeatureFlagEnabled("gerrit") can
return undefined and allow the downstream fallback pattern
(useFeatureFlagEnabled("gerrit") ?? true) to work as intended.
In `@fdm-app/app/routes/farm`.$b_id_farm.$calendar.gerrit.tsx:
- Around line 43-44: Server-side authorization in the action function
incorrectly uses the browser PostHog SDK (imported as posthog from "posthog-js")
and calls posthog.featureFlags.isFeatureEnabled("gerrit"), which returns
undefined in a server action; replace that usage by importing and using the
server PostHog client from the existing posthog.server.ts and call its feature
flag check scoped to the current user (e.g., get the server client and invoke
featureFlags.isFeatureEnabled or equivalent on that instance inside the action
function), removing the client-side posthog import and ensuring the check uses
the request/user context so the boolean result is reliable.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 09275ea2-1023-4353-82f9-2cb18f3faaf9
📒 Files selected for processing (3)
fdm-app/app/components/blocks/sidebar/farm.tsxfdm-app/app/root.tsxfdm-app/app/routes/farm.$b_id_farm.$calendar.gerrit.tsx
| bootstrap: { | ||
| featureFlags: { | ||
| gerrit: false, | ||
| }, | ||
| }, |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In posthog-js, when posthog.initusesbootstrap.featureFlags = { gerrit: false }, does useFeatureFlagEnabled("gerrit")returnfalse(instead ofundefined) before/if remote flags are missing?
💡 Result:
Yes, when posthog.init is called with bootstrap.featureFlags = { gerrit: false }, the useFeatureFlagEnabled("gerrit") hook returns false (instead of undefined) immediately after initialization, before remote flags are fetched, assuming the bootstrapped flags are properly loaded.
Citations:
- 1: https://posthog.com/docs/feature-flags/bootstrapping
- 2: Bootstrapped inactive feature flags are ignored PostHog/posthog-js#859
- 3: useFeatureFlagEnabled returns an incorrect value when flag is enabled PostHog/posthog-js#1714
- 4: getFeatureFlag returns undefined for bootstrapped feature flag PostHog/posthog-js#1556
Remove the gerrit bootstrap to restore intended default-allow behavior.
At lines 98–102, bootstrapping gerrit: false causes useFeatureFlagEnabled("gerrit") to return false instead of undefined. This breaks the downstream pattern useFeatureFlagEnabled("gerrit") ?? true, which relies on undefined to default the flag to enabled. When the bootstrap value is present, the fallback never executes, and Gerrit remains disabled even when the flag should default to allowed.
Suggested fix
posthog.init(posthogConfig.key, {
api_host: "/ingest",
ui_host: posthogConfig.host,
person_profiles: "always",
- bootstrap: {
- featureFlags: {
- gerrit: false,
- },
- },
loaded: () => {},
})📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| bootstrap: { | |
| featureFlags: { | |
| gerrit: false, | |
| }, | |
| }, |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@fdm-app/app/root.tsx` around lines 98 - 102, The bootstrap is explicitly
setting featureFlags.gerrit to false which causes
useFeatureFlagEnabled("gerrit") to return false instead of undefined; remove the
"gerrit: false" entry from the bootstrap.featureFlags object (or omit the gerrit
key entirely) so useFeatureFlagEnabled("gerrit") can return undefined and allow
the downstream fallback pattern (useFeatureFlagEnabled("gerrit") ?? true) to
work as intended.
There was a problem hiding this comment.
I added this so that it forcibly disables the feature until the feature flag value true or undefined is returned from PostHog servers or the PostHog cache.
There was a problem hiding this comment.
@BoraIneviNMI, thanks for the clarification — that makes the design clear. The two patterns serve distinct scenarios:
- PostHog configured (production): Bootstrap starts at
falseto keep Gerrit disabled during the initialization window, then PostHog's remote value takes over once loaded. - PostHog not configured (development):
useFeatureFlagEnabled("gerrit")returnsundefined(PostHog is never initialized), so?? truekicks in and defaults to enabled.
The ?? true fallback is not meant to handle the pre-load window, but rather the no-PostHog environment entirely. That's a coherent design. My concern was incorrect — I'll withdraw it.
One thing worth keeping in mind: if the gerrit flag doesn't exist at all in the PostHog dashboard (as opposed to being explicitly disabled), PostHog may return undefined after loading, which would trigger ?? true even in production. That may or may not be the intended behavior, but it aligns with the PR description's "assume enabled when the flag is missing" posture.
( •ᴗ• ) ∫
✏️ Learnings added
Learnt from: BoraIneviNMI
Repo: nmi-agro/fdm PR: 545
File: fdm-app/app/root.tsx:98-102
Timestamp: 2026-04-01T15:34:57.962Z
Learning: In `fdm-app/app/root.tsx`, the PostHog `bootstrap.featureFlags.gerrit: false` is intentional. It disables the Gerrit feature during the PostHog initialization window in production. The client-side `useFeatureFlagEnabled("gerrit") ?? true` pattern is separately intended for development environments where PostHog is not configured (hook returns `undefined`). These two patterns serve distinct scenarios and should not be conflated.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: SvenVw
Repo: nmi-agro/fdm PR: 535
File: fdm-app/package.json:57-57
Timestamp: 2026-03-31T07:54:00.823Z
Learning: When reviewing files that import icons from `lucide-react` (v1.x, including v1.7.0), treat both export styles as valid: the base icon name (e.g., `Loader2`) and the `Icon`-suffixed alias (e.g., `Loader2Icon`). Do not flag `Icon`-suffixed imports from `lucide-react` as incorrect.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.changeset/little-lemons-lead.md:
- Line 5: The release note text contains the non-hyphenated compound adjective
"server side" in the string "Added server side authorization for Gerrit.";
update that phrase to use the hyphenated form "server-side" in the
.changeset/little-lemons-lead.md release note so the user-facing changelog reads
"Added server-side authorization for Gerrit." and preserves surrounding
capitalization and punctuation.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 8ca1fd86-1bb7-4bc9-8f4a-37f01a13d1ce
📒 Files selected for processing (2)
.changeset/little-lemons-lead.mdfdm-app/app/routes/farm.$b_id_farm.$calendar.gerrit.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- fdm-app/app/routes/farm.$b_id_farm.$calendar.gerrit.tsx
2 participants
Bug fixes
Summary by CodeRabbit
Chores
Bug Fixes / Access Control