Skip to content

add authentication in app settings#791

Open
sadakchap wants to merge 6 commits intoproduction2from
1206/authentication-settings
Open

add authentication in app settings#791
sadakchap wants to merge 6 commits intoproduction2from
1206/authentication-settings

Conversation

@sadakchap
Copy link
Copy Markdown

No description provided.

@sadakchap sadakchap requested a review from nataliaconde March 10, 2026 02:10
@alyssoncm
Copy link
Copy Markdown

@claude review this PR

@claude
Copy link
Copy Markdown

claude bot commented Mar 16, 2026

Claude finished @alyssoncm's task in 1m 28s —— View job


PR Review: Add Authentication in App Settings

  • Gather context and read changed files
  • Review SocialAuth.react.js (main component)
  • Review SocialAuth.scss
  • Review ParseApp.js changes
  • Review Dashboard.js and DashboardView.react.js changes
  • Review serverInfo.js changes
  • Post final review

Overall, the PR introduces a functional OAuth/Social Auth settings page with a clean structure. A few issues worth addressing:


Bugs & Issues

1. Deprecated lifecycle method
[SocialAuth.react.js:106] — componentWillMount was deprecated in React 16.3 and removed in React 18. Use componentDidMount instead.

// ❌
componentWillMount() {
  this.loadData();
}

// ✅
componentDidMount() {
  this.loadData();
}

Fix this →


2. VKontakte field naming inconsistency
[SocialAuth.react.js:370] — The VKontakte Application ID field uses the key appIds (plural), but it stores a single string value (unlike Facebook's appIds which is a proper array). This is semantically misleading.

value={vkontakte.appIds ?? ''}  // appIds implies array, but it's a string

The name should be clarified — either rename the key to appId (singular) or document that it's a string. The renderOauthFooterChanges helper also compares it as a string (curVk.appIds || '') which is inconsistent with the Facebook approach.


3. Subtle validate return value
[SocialAuth.react.js:440] — validate={() => ''} returns an empty string to signal "no error". An empty string is falsy but semantically unclear. Prefer returning null or just omitting the validator if no validation is needed.


4. Fragile setTimeout after save
[SocialAuth.react.js:438] — Using a hardcoded 1200ms delay to reset the form after save is fragile and timing-dependent. If the save success animation takes longer or shorter, the UX degrades. Consider tying the reset to the UI state instead.

afterSave={({ fields, resetFields }) => {
  this.setState({ ... });
  setTimeout(() => resetFields(), 1200); // fragile timing
}}

5. No Facebook App ID format validation
[SocialAuth.react.js:135-149] — handleFbAddAppId only checks for duplicates and empty input. Facebook App IDs are numeric strings; a simple /^\d+$/.test(input) check would prevent users from entering clearly invalid values.


Minor / Style

6. serverInfo.js'Authentication' not listed as an allowed route
[src/lib/serverInfo.js:27-28] — The nav link uses name: 'Authentication' but only 'social-auth' and 'Social Auth' are added to ALWAYS_ALLOWED_ROUTES. Other entries list both the route name and the display name (e.g., 'Environment Variables' / 'environment-variables'). Consider adding 'Authentication' for consistency, or verify it's not needed by the canAccess logic.

7. b4aSettings global with eslint suppression
[src/lib/ParseApp.js:1876, 1890] — // eslint-disable-next-line no-undef is used twice for b4aSettings. This pattern exists elsewhere in the codebase so it's not blocking, but it's worth tracking as a cleanup opportunity.

8. Toolbar renders empty
[SocialAuth.react.js:129-133] — renderToolbar() returns a <Toolbar> with no children. If no actions/buttons are expected, this is fine, but it's worth verifying this is intentional and won't render an empty bar visually.


Summary

Critical componentWillMount → must fix before merge
Recommended Facebook ID validation, VKontakte naming, validate return
Minor setTimeout, toolbar children, serverInfo consistency

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.

2 participants