Snapshot subscription frontend - #5136
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds a complete snapshot subscription settings page with GraphQL queries and mutations, chapter and project preference controls, navigation wiring, frontend tests, accessibility coverage, and backend tests for related models, admin configuration, GraphQL operations, and mutation flows. Updates the snapshot Makefile command target. ChangesBackend subscription coverage
Frontend subscription settings
Snapshot command wiring
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## feature/community-snapshots #5136 +/- ##
===============================================================
- Coverage 98.77% 98.70% -0.07%
===============================================================
Files 543 547 +4
Lines 17443 17859 +416
Branches 2539 2607 +68
===============================================================
+ Hits 17229 17628 +399
- Misses 88 92 +4
- Partials 126 139 +13
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report in Codecov by Harness.
🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@backend/apps/owasp/api/internal/mutations/snapshot_subscription.py`:
- Around line 98-123: The chapter assignment path is unguarded, so invalid
chapter ids can raise an IntegrityError or be misreported as a duplicate
subscription. Add and use the shared guarded helper (for example, _set_chapters)
from _apply_subscription_preferences, and replace the direct
subscription.chapters.set(...) calls in create_snapshot_subscription and
update_snapshot_subscription so invalid ids are converted to ValueError and
handled by the existing error paths.
- Around line 138-142: The frequency validation is duplicated in
create_snapshot_subscription and update_snapshot_subscription, using the same
SnapshotSubscription.Frequency.choices membership check. Replace the manual
string validation with a Strawberry enum derived from
SnapshotSubscription.Frequency and use it on the input types so schema-level
type safety handles invalid values. Remove the duplicated validation branches in
create_snapshot_subscription and update_snapshot_subscription and update any
related input handling to work with the enum value directly.
- Around line 59-96: _sync_project_preferences is doing one
ProjectSubscriptionPreference.objects.create() per preference and there is no
upper bound on list inputs, so update this flow to batch writes with bulk_create
after the duplicate-project-id check and add a hard cap for preference/chapter
ID lists similar to normalize_limit/MAX_LIMIT. Apply the same size limit to
UpdateSnapshotSubscriptionInput and subscribed_chapter_ids handling, and keep
the invalid-project validation behavior intact in _sync_project_preferences and
related snapshot subscription mutation paths.
In `@backend/apps/owasp/models/project_subscription_preference.py`:
- Around line 9-19: Add an explicit default ordering to the Meta options in
ProjectSubscriptionPreference so `subscription.project_preferences.all()`
returns a stable order. Update the `Meta` class on
`ProjectSubscriptionPreference` to include an `ordering` that sorts by a
deterministic field (for example the related project or primary key), and keep
the existing `UniqueConstraint` intact.
In `@frontend/__tests__/a11y/pages/SettingsPage.a11y.test.tsx`:
- Around line 38-59: The current SettingsPage a11y test only checks the default
non-subscribed render, so the suggestions dropdown state is never exercised.
Update the existing SettingsPage.a11y.test.tsx coverage to use the rendered
SettingsPage interaction path for the project/chapter search input, type at
least 3 characters to open EntityPicker suggestions, and run axe against that
open dropdown state. Keep the existing console.error suppression and use the
same mocked useSession and useQuery setup so the new assertion covers the
interactive suggestions UI.
In `@frontend/__tests__/unit/pages/SettingsPage.test.tsx`:
- Around line 204-214: The current SettingsPage tests only verify UI toggling
and do not validate the mutation payload produced by getMutationVariables in
SettingsPage/page.tsx. Extend the subscription/save flow tests in
SettingsPage.test.tsx to click Subscribe or Save Changes and assert
mockCreateMutation or mockUpdateMutation receives the expected variables object,
including frequency, subscribedChapterIds, and projectPreferences mapping. Use
the existing setupMocks, render, and mutation mocks to cover the id-parsing and
field-mapping behavior so regressions in getMutationVariables are caught.
In `@frontend/src/app/settings/page.tsx`:
- Around line 82-136: The debounced suggestion fetch in EntityPicker is being
recreated whenever selectedItems changes, which resets the debounce timer on
every parent render. Move the debounce wrapper so it is created once from stable
dependencies like client, searchQuery, and searchResultKey, and read the latest
selectedItems through a ref inside the callback instead of including it in the
dependency list. Keep the existing fetchSuggestions and useEffect flow, but
ensure the debounced function instance stays stable across renders.
- Line 267: The frequency state in the settings page is too broadly typed as a
plain string even though only weekly and monthly are valid options. Update the
useState declaration for frequency in the settings page component to use the
narrower union type that matches FrequencySelector and getMutationVariables, so
invalid assignments are caught by TypeScript.
- Around line 265-561: `SubscriptionContent` defines `FrequencySelector`,
`GlobalContentPreferences`, `ProjectSubscriptions`, and `ChapterFilters` inline,
which causes React to remount them and reset nested `EntityPicker` state on
every parent render. Hoist these into stable top-level components (or memoized
components) and pass the needed state/handlers as props so their identities
don’t change when `SubscriptionContent` updates. Keep the existing logic in
`SubscriptionContent`, but render the extracted components there instead of
recreating them on each render.
In `@frontend/src/server/queries/subscriptionQueries.ts`:
- Around line 3-99: The subscription selection set is duplicated in
GET_MY_SUBSCRIPTION, CREATE_SNAPSHOT_SUBSCRIPTION, and
UPDATE_SNAPSHOT_SUBSCRIPTION, so extract the repeated
subscription/projectPreferences/chapters fields into a shared GraphQL fragment
and reuse it in all three operations. Place the fragment alongside the existing
gql definitions in subscriptionQueries.ts, and make sure its on <TypeName>
clause matches the backend subscription node type (for example
SnapshotSubscriptionNode) before applying it to both mutations and the query.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: ccd0a400-394a-4cdb-baa4-4566737fef0e
⛔ Files ignored due to path filters (3)
frontend/src/types/__generated__/chapterQueries.generated.tsis excluded by!**/__generated__/**frontend/src/types/__generated__/graphql.tsis excluded by!**/__generated__/**frontend/src/types/__generated__/subscriptionQueries.generated.tsis excluded by!**/__generated__/**
📒 Files selected for processing (32)
backend/apps/owasp/Makefilebackend/apps/owasp/admin/snapshot_subscription.pybackend/apps/owasp/api/internal/mutations/__init__.pybackend/apps/owasp/api/internal/mutations/snapshot_subscription.pybackend/apps/owasp/api/internal/nodes/chapter.pybackend/apps/owasp/api/internal/nodes/project_subscription_preference.pybackend/apps/owasp/api/internal/nodes/snapshot_subscription.pybackend/apps/owasp/api/internal/queries/__init__.pybackend/apps/owasp/api/internal/queries/chapter.pybackend/apps/owasp/api/internal/queries/snapshot_subscription.pybackend/apps/owasp/migrations/0076_add_project_subscription_preference.pybackend/apps/owasp/models/__init__.pybackend/apps/owasp/models/project_subscription_preference.pybackend/apps/owasp/models/snapshot_subscription.pybackend/data/nest.dumpbackend/settings/graphql.pybackend/tests/unit/apps/owasp/admin/snapshot_subscription_test.pybackend/tests/unit/apps/owasp/api/internal/mutations/__init__.pybackend/tests/unit/apps/owasp/api/internal/mutations/snapshot_subscription_test.pybackend/tests/unit/apps/owasp/api/internal/nodes/project_subscription_preference_test.pybackend/tests/unit/apps/owasp/api/internal/nodes/snapshot_subscription_test.pybackend/tests/unit/apps/owasp/api/internal/queries/chapter_test.pybackend/tests/unit/apps/owasp/api/internal/queries/snapshot_subscription_test.pybackend/tests/unit/apps/owasp/models/project_subscription_preference_test.pybackend/tests/unit/apps/owasp/models/snapshot_subscription_test.pyfrontend/__tests__/a11y/pages/SettingsPage.a11y.test.tsxfrontend/__tests__/mockData/mockSubscriptionData.tsfrontend/__tests__/unit/pages/SettingsPage.test.tsxfrontend/src/app/settings/page.tsxfrontend/src/components/UserMenu.tsxfrontend/src/server/queries/chapterQueries.tsfrontend/src/server/queries/subscriptionQueries.ts
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
All reported issues were addressed across 35 files
Tip: cubic can generate docs of your entire codebase and keep them up to date. Try it here.
Re-trigger cubic
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
frontend/src/app/settings/page.tsx (2)
636-745: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicated block across active/not-subscribed branches.
FrequencySelector,GlobalContentPreferences,ProjectSubscriptions, andChapterFiltersare rendered with identical props in both thehasActiveSubscriptionand not-subscribed branches (lines 653-673 vs 711-731). Extracting this shared block once (before the conditional, with only the status card/action buttons varying) would remove the duplication.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/src/app/settings/page.tsx` around lines 636 - 745, The settings page duplicates the same subscription-configuration UI in both the active and not-subscribed branches. Refactor the shared `FrequencySelector`, `GlobalContentPreferences`, `ProjectSubscriptions`, and `ChapterFilters` block in `page.tsx` into a single rendered section outside the `hasActiveSubscription` conditional, and keep only the status card plus the action buttons (`handleUpdate`/`handleCancel` vs `handleSubscribe`) inside the branches.
39-73: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse the generated subscription GraphQL types here.
frontend/src/app/settings/page.tsxalready has generated equivalents infrontend/src/types/__generated__/subscriptionQueries.generated.ts; switching to those would remove the handwritten response interfaces and thekeyof typeof datacast onclient.query.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/src/app/settings/page.tsx` around lines 39 - 73, Replace the handwritten subscription response interfaces in settings/page.tsx with the generated GraphQL types from subscriptionQueries.generated.ts, and update the query/mutation code to use those types directly. In particular, remove the local EntityItem/ProjectPreference/SubscriptionData/GetSubscriptionResponse/MutationResponse definitions, import the generated equivalents, and type the client.query and mutation responses so you no longer need the keyof typeof data cast.Source: Learnings
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@frontend/__tests__/a11y/pages/SettingsPage.a11y.test.tsx`:
- Around line 36-38: The test cleanup in SettingsPage.a11y.test.tsx only
restores spies, so the useApolloClient mock override created in the “open
suggestions dropdown” test can leak into later tests and describe.each theme
iterations. Update the afterEach cleanup to also reset the mocked
useApolloClient/jest.fn state (including any per-test mockImplementation set via
mockClientQuery) so each test starts with the default mock behavior. Keep the
fix scoped to the existing test helpers and the useApolloClient mock setup.
In `@frontend/src/app/settings/page.tsx`:
- Around line 419-449: The Chapter subscriptions toggle is only hiding the UI in
ChapterFilters but not clearing the underlying selection, so stale subscribed
chapter IDs still flow through getMutationVariables. Update the
toggleGlobalPreference path (and any related ChapterFilters state handling) to
reset selectedChapters when includeChapters is turned off, and ensure the
EntityPicker-backed chapter selection cannot persist invisibly after disabling
chapters.
- Around line 157-176: The suggestion items in renderSuggestions currently
select only via onMouseDown, so keyboard and screen-reader activation on the
button elements does not trigger handleSelect. Keep the existing onMouseDown
behavior to prevent the blur/close race, but also wire handleSelect through
onClick on each suggestion button so Enter/Space activation works. Use the
renderSuggestions and handleSelect symbols to update the suggestion list
behavior, and add a keyboard-selection regression test in SettingsPage.test.tsx
that tabs to a suggestion and activates it with Enter.
---
Outside diff comments:
In `@frontend/src/app/settings/page.tsx`:
- Around line 636-745: The settings page duplicates the same
subscription-configuration UI in both the active and not-subscribed branches.
Refactor the shared `FrequencySelector`, `GlobalContentPreferences`,
`ProjectSubscriptions`, and `ChapterFilters` block in `page.tsx` into a single
rendered section outside the `hasActiveSubscription` conditional, and keep only
the status card plus the action buttons (`handleUpdate`/`handleCancel` vs
`handleSubscribe`) inside the branches.
- Around line 39-73: Replace the handwritten subscription response interfaces in
settings/page.tsx with the generated GraphQL types from
subscriptionQueries.generated.ts, and update the query/mutation code to use
those types directly. In particular, remove the local
EntityItem/ProjectPreference/SubscriptionData/GetSubscriptionResponse/MutationResponse
definitions, import the generated equivalents, and type the client.query and
mutation responses so you no longer need the keyof typeof data cast.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 0cdbcc1f-5513-4d1c-bebf-fefea0599867
⛔ Files ignored due to path filters (1)
frontend/src/types/__generated__/subscriptionQueries.generated.tsis excluded by!**/__generated__/**
📒 Files selected for processing (4)
frontend/__tests__/a11y/pages/SettingsPage.a11y.test.tsxfrontend/__tests__/unit/pages/SettingsPage.test.tsxfrontend/src/app/settings/page.tsxfrontend/src/server/queries/subscriptionQueries.ts
There was a problem hiding this comment.
6 issues found across 5 files (changes from recent commits).
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
frontend/__tests__/a11y/pages/SettingsPage.a11y.test.tsx (1)
36-38: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
clearAllMocks()doesn’t reset the leaked Apollo implementation.This still leaves the
useApolloClient.mockReturnValue(...)from the dropdown test installed for later cases;clearAllMocks()only clears call history. Reset the mock implementation or reapply the default inbeforeEach.🧪 Proposed fix
beforeEach(() => { ;(useTheme as jest.Mock).mockReturnValue({ theme, setTheme: jest.fn() }) + ;(useApolloClient as jest.Mock).mockReturnValue({ + query: jest.fn().mockResolvedValue({ data: { searchProjects: [], searchChapters: [] } }), + }) document.documentElement.classList.toggle('dark', theme === 'dark') }) afterEach(() => { jest.restoreAllMocks() - jest.clearAllMocks() + jest.resetAllMocks() })#!/bin/bash # Verify cleanup/reset behavior locations in this test file. rg -n "clearAllMocks|resetAllMocks|useApolloClient.*mockReturnValue" frontend/__tests__/a11y/pages/SettingsPage.a11y.test.tsx🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/__tests__/a11y/pages/SettingsPage.a11y.test.tsx` around lines 36 - 38, The test cleanup in SettingsPage.a11y.test.tsx is only clearing call history, so the mocked useApolloClient implementation from the dropdown case can leak into later tests. Update the afterEach cleanup around jest.restoreAllMocks/jest.clearAllMocks to also reset the mock implementation, or reestablish the default useApolloClient behavior in beforeEach so each test starts clean.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@frontend/__tests__/a11y/pages/SettingsPage.a11y.test.tsx`:
- Line 118: The SettingsPage accessibility test is only checking for suggestion
text, so it can pass even if the autocomplete popup lacks proper semantics.
Update the test around the OWASP ZAP suggestion to assert the accessible popup
structure exposed by the suggestions UI, using the relevant listbox/option roles
from the component instead of just text. This should be done in
SettingsPage.a11y.test.tsx against the same suggestion interaction so the test
guards the accessible roles provided by the autocomplete/popover implementation.
In `@frontend/__tests__/unit/pages/SettingsPage.test.tsx`:
- Around line 13-16: The mutation stub logic in SettingsPage.test.tsx is
treating any non-create/update document as a cancel action, which can hide
wiring regressions. Update the test setup that branches on
CREATE_SNAPSHOT_SUBSCRIPTION and UPDATE_SNAPSHOT_SUBSCRIPTION to explicitly
import and handle CANCEL_SNAPSHOT_SUBSCRIPTION, and make the fallback path throw
for any unknown mutation document instead of defaulting to cancel. Ensure the
same strict handling is applied in the affected helper/mock code near the
referenced test blocks so unexpected documents fail fast.
In `@frontend/src/app/settings/page.tsx`:
- Line 217: The suggestions popup is only using aria-label on a plain div, so it
is not exposed as an autocomplete popup connected to the search input. Update
the suggestions container in the settings page to use proper combobox/listbox
semantics tied to the input, and ensure the input references the popup
appropriately. If you adopt the listbox pattern in the suggestions rendering,
also mark each suggestion item as an option so assistive tech can navigate it
correctly.
- Line 628: The error handling in the settings page currently gates on the
top-level data object, which can hide a failed `mySubscription` result when
Apollo returns partial data. Update the conditional around the subscription
error card in the `page.tsx` settings component to check the `subscription`
value instead of `data`, so the error state still renders when `mySubscription`
is null even if `errorPolicy: 'all'` returns other data.
---
Duplicate comments:
In `@frontend/__tests__/a11y/pages/SettingsPage.a11y.test.tsx`:
- Around line 36-38: The test cleanup in SettingsPage.a11y.test.tsx is only
clearing call history, so the mocked useApolloClient implementation from the
dropdown case can leak into later tests. Update the afterEach cleanup around
jest.restoreAllMocks/jest.clearAllMocks to also reset the mock implementation,
or reestablish the default useApolloClient behavior in beforeEach so each test
starts clean.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: f4f1cbac-2b30-4dcb-852f-87906f16a441
📒 Files selected for processing (3)
frontend/__tests__/a11y/pages/SettingsPage.a11y.test.tsxfrontend/__tests__/unit/pages/SettingsPage.test.tsxfrontend/src/app/settings/page.tsx
There was a problem hiding this comment.
1 issue found across 3 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
frontend/src/app/settings/page.tsx (1)
610-622: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winUse numeric IDs from the search results. In
frontend/src/app/settings/page.tsx:610-622,searchProjects/searchChaptersreturn GraphQL node IDs as strings, soNumber.parseInt(...)can yieldNaNand send invalidsubscribedChapterIds/projectIdvalues. Query a dedicated integer field instead of parsingid.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/src/app/settings/page.tsx` around lines 610 - 622, The mutation payload in getMutationVariables is converting GraphQL node IDs with Number.parseInt, which can produce invalid values for subscribedChapterIds and projectPreferences.projectId. Update the selected chapter/project data used by selectedChapters and projectPreferences to carry the dedicated numeric ID field from searchProjects/searchChapters, then use that numeric field directly in getMutationVariables instead of parsing c.id or p.project.id.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@frontend/src/app/settings/page.tsx`:
- Around line 610-622: The mutation payload in getMutationVariables is
converting GraphQL node IDs with Number.parseInt, which can produce invalid
values for subscribedChapterIds and projectPreferences.projectId. Update the
selected chapter/project data used by selectedChapters and projectPreferences to
carry the dedicated numeric ID field from searchProjects/searchChapters, then
use that numeric field directly in getMutationVariables instead of parsing c.id
or p.project.id.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 9f4f4fc0-d0c2-424a-9cff-31e4af3de364
📒 Files selected for processing (3)
frontend/__tests__/a11y/pages/SettingsPage.a11y.test.tsxfrontend/__tests__/unit/pages/SettingsPage.test.tsxfrontend/src/app/settings/page.tsx
7b0ebce
|
Hi @kasya, please review it and let me know if any changes are required. |
HarshitVerma109
added
the
gsoc2026:harshitverma109
kasya
left a comment
kasya
left a comment
There was a problem hiding this comment.
@HarshitVerma109 hi! Great work overall! 👍🏼
I left some requests bellow ⬇️
There was a problem hiding this comment.
1 issue found across 3 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="frontend/src/components/ActionButton.tsx">
<violation number="1" location="frontend/src/components/ActionButton.tsx:11">
P2: `isDisabled` is added to the component API but only handled in the `<Button>` rendering path — the `<Link>` path (when `url` is set) silently ignores it, making the prop unreliable. Either handle it in both branches or scope the API so it's clear `isDisabled` only applies to the button variant.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
There was a problem hiding this comment.
All reported issues were addressed across 3 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
There was a problem hiding this comment.
All reported issues were addressed across 6 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
There was a problem hiding this comment.
0 issues found across 3 files (changes from recent commits).
Confidence score: 5/5
- Automated review surfaced no issues in the provided summaries.
- No files require special attention.
Requires human review: Auto-approval blocked by 2 unresolved issues from previous reviews.
Re-trigger cubic
There was a problem hiding this comment.
0 issues found across 1 file (changes from recent commits).
Confidence score: 5/5
- Automated review surfaced no issues in the provided summaries.
- No files require special attention.
Requires human review: Auto-approval blocked by 2 unresolved issues from previous reviews.
Re-trigger cubic
|
Hi @kasya, please review it and let me know if any changes are required. |
kasya
left a comment
kasya
left a comment
There was a problem hiding this comment.
@HarshitVerma109 this is very close to be done! 👍🏼 Looks much better now.
Left some comments ⬇️
There was a problem hiding this comment.
All reported issues were addressed across 2 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
There was a problem hiding this comment.
0 issues found across 1 file (changes from recent commits).
Confidence score: 5/5
- Automated review surfaced no issues in the provided summaries.
- No files require special attention.
Requires human review: Auto-approval blocked by 2 unresolved issues from previous reviews.
Re-trigger cubic
|
Hi @kasya, please review it and let me know if any changes are required. |
kasya
left a comment
kasya
left a comment
There was a problem hiding this comment.
@HarshitVerma109 This looks good 👍🏼
One thing I noticed - name seems to be optional, and I think we discussed this before.
The thing with how it is setup right now, if user does not add name we show something like this on their settings page:
And like this when trying to add to an existing sub:
These views differ. And a bigger problem - because names must be unique per user and the name defaults to empty, a user can only ever have one unnamed subscription. Creating a second one without typing a name fails with "A subscription with this name already exists" - which is a confusing message if you didn't realize blank counts as a name.
Three ways out, roughly in order of how little they change:
- Generate a default in the mutation when name is blank — e.g. "Subscription 3" or seed it from the frequency/first picked entity. Keeps the constraint meaningful - I think this was what I suggested originally.
- Make name required in both create forms. Simplest, but forces naming on the one-click Subscribe flow, which is the flow most likely to want a blank.
- Make the constraint ignore blanks - store NULL instead of "" (Postgres treats NULLs as distinct in unique constraints). Needs a migration and a model change in this case.
|
@HarshitVerma109 Left question/comment for you ⬆️ |
Signed-off-by: Harsh <harshit1092004@gmail.com>
Signed-off-by: Harsh <harshit1092004@gmail.com>
Signed-off-by: Harsh <harshit1092004@gmail.com>
Signed-off-by: Harsh <harshit1092004@gmail.com>
88b3721 to
9ac7dc8
Compare
There was a problem hiding this comment.
0 issues found across 1 file (changes from recent commits).
Confidence score: 5/5
- Automated review surfaced no issues in the provided summaries.
- No files require special attention.
Requires human review: Auto-approval blocked by 2 unresolved issues from previous reviews.
Re-trigger cubic
Signed-off-by: Harsh <harshit1092004@gmail.com>
|
There was a problem hiding this comment.
All reported issues were addressed across 1 file (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
|
Hi @kasya, please review it and let me know if changes are required. |
3 participants
Proposed change
Resolves #5135
This PR adds the frontend
/settingspage to allow users to subscribe to snapshot digests and manage their preferencesChecklist