feat: implement shared element transitions, responsive event card fee…#648
Open
Bhavikapatel06 wants to merge 4 commits into
Open
Conversation
…d grid, and centered slim footer
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
💤 Files with no reviewable changes (1)
📝 WalkthroughWalkthroughThe PR migrates the app's stack navigator to ChangesApp UI and Navigation
Cloud Functions Logic and Tooling
Sequence Diagram(s)sequenceDiagram
participant UserFeed
participant EventCard
participant NativeStack
rect rgba(99, 179, 237, 0.5)
Note over UserFeed,EventCard: Shared Transition Setup
UserFeed->>EventCard: renders Animated.Image sharedTransitionTag=event.id
end
EventCard->>NativeStack: navigate to EventDetail
rect rgba(154, 230, 180, 0.5)
Note over NativeStack: Native Stack Transition
NativeStack->>NativeStack: apply shared element transition
end
sequenceDiagram
participant Caller
participant checkAndUpdateRateLimit
participant Firestore
participant logger
participant emailSender
Caller->>checkAndUpdateRateLimit: call(userId, isEventCreation=true)
checkAndUpdateRateLimit->>Firestore: runTransaction(tx)
Firestore->>Firestore: read rate limit doc
alt daily limit exceeded
Firestore->>logger: logEntry(suspicious activity) [inside tx]
Firestore->>emailSender: sendEmail(admin alert) [inside tx]
Firestore-->>checkAndUpdateRateLimit: return 429 response
else within limit
Firestore->>Firestore: increment counter
Firestore-->>checkAndUpdateRateLimit: return 200 response
end
checkAndUpdateRateLimit-->>Caller: response
Estimated code review effort🎯 4 (Complex) | ⏱️ ~65 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/src/components/CustomTabBar.js (1)
70-80:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winIncrease effective touch target size for tab buttons.
On Line 70 and Lines 130-131, each tab is effectively
36x36, which is small for touch interaction and can cause missed taps. Keep the visual size, but expand the tappable area.Proposed fix
<TouchableOpacity key={route.key} accessible={true} accessibilityRole="button" accessibilityState={isFocused ? { selected: true } : {}} accessibilityLabel={ options.tabBarAccessibilityLabel || `${route.name} tab` } testID={options.tabBarTestID} onPress={onPress} style={styles.tabItem} + hitSlop={{ top: 6, bottom: 6, left: 6, right: 6 }} >tabItem: { alignItems: 'center', justifyContent: 'center', - height: 36, - width: 36, + height: 36, + width: 36, },Also applies to: 127-131
🤖 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 `@app/src/components/CustomTabBar.js` around lines 70 - 80, The TouchableOpacity component wrapping the tab buttons at line 70 and the similar component at lines 127-131 has an effective touch target size of 36x36 pixels, which is too small for reliable touch interaction. Add the hitSlop prop to the TouchableOpacity component to expand the tappable area in all directions (top, bottom, left, right) while keeping the visual size unchanged. This will make the tab buttons easier to tap for users without affecting the UI appearance.
🧹 Nitpick comments (2)
cloud-functions/seed-events.js (1)
65-67: ⚡ Quick winMake seed IDs deterministic for repeatable local runs.
Using
doc()creates new IDs every run, so local datasets grow with duplicates and make manual QA less reliable. Prefer fixed IDs (or an upsert strategy) for stable emulator state.🤖 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 `@cloud-functions/seed-events.js` around lines 65 - 67, The current implementation in the mockEvents.forEach loop uses db.collection('events').doc() which generates random document IDs on every execution, causing duplicate documents and unreliable emulator state. Replace the random ID generation by either using fixed/deterministic IDs derived from the event data (such as a unique property or hash) or implementing an upsert strategy. Ensure the ref variable is assigned a consistent ID before calling batch.set(ref, event) so that repeated seed runs maintain stable, non-duplicative data.cloud-functions/lib/branchReport.js (1)
286-290: 💤 Low valueAttendance from unregistered branches is now silently ignored.
The change only increments attendance when a stats entry already exists (from registrations). Check-ins from users whose branch doesn't match any registered branch will be silently dropped.
This may be intentional to ensure
attendanceRatedoesn't exceed 100%, but it could also cause legitimate attendance to be uncounted if:
- User's branch differs between registration and check-in
- Branch name variants exist (e.g., "CS" vs "Computer Science")
If this behavior is intentional, consider adding a log for visibility when check-ins are skipped.
🤖 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 `@cloud-functions/lib/branchReport.js` around lines 286 - 290, The current code in the statsByBranch block only increments attendance when a stats entry already exists for the branch. This means check-ins from users whose branch doesn't have a registered stats entry are silently dropped. To fix this, either create a new stats entry for unregistered branches (following the same structure with attendance initialized to 1 and events as a Set containing eventDoc.id) in an else clause after the if (stats) block, or add a log statement to track when check-ins from unregistered branches are skipped. This will either ensure all legitimate attendance is counted or provide visibility into why some check-ins are being ignored.
🤖 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 `@cloud-functions/lib/clubReputation.js`:
- Around line 117-118: The validation check for clubRating in the condition
around line 117-118 only enforces a lower bound by rejecting values <= 0, but
lacks an upper bound validation. This allows arbitrarily large rating values to
be accepted and inflated into reputation scores. Add an upper bound check to the
existing condition that also rejects clubRating values that exceed a reasonable
maximum threshold (such as 5 or 10), ensuring the function returns null for both
invalid low and invalid high values just as it currently does for the lower
bound.
In `@cloud-functions/lib/dedicatedStudentCertificate.js`:
- Around line 117-122: The console.log and console.warn statements in the
dedicatedStudentCertificate.js file are logging the full userId without masking,
which violates privacy/compliance requirements. To fix this, mask the userId in
all logging statements (in the console.log at line 117, console.warn at line
121, and the additional location at line 166) by using the pattern
userId.slice(0, 4) + '***' instead of the full userId. This will display only
the first 4 characters followed by asterisks, protecting user privacy while
still allowing enough information for debugging purposes.
In `@cloud-functions/lib/logger.js`:
- Around line 70-72: The unguarded JSON.stringify(error) call in the else if
block (where error is an object type) can throw on circular references, BigInt
values, or problematic toJSON() methods, causing the logError function itself to
crash. Wrap the JSON.stringify(error) assignment in a try/catch block, and
provide a safe fallback in the catch handler (such as using error.toString() or
a descriptive error message) to ensure the logging utility remains defensive and
never propagates exceptions.
In `@cloud-functions/lib/permanentCleanup.js`:
- Around line 67-69: The batch.update() call that decrements eventCount will
fail with NOT_FOUND if the owner document doesn't exist, preventing the event
deletion from completing. Before calling batch.update() on the ownerRef, first
fetch the owner document using ownerRef.get() to check if it exists, and only
include the batch.update() call with the eventCount increment if the owner
document snapshot exists. This ensures that orphaned events can still be cleaned
up even if the owner account has been deleted.
In `@cloud-functions/lib/utils/rateLimiter.js`:
- Around line 138-161: The logEntry and sendEmail side effects are currently
inside the Firestore transaction callback, which means they will execute
multiple times if the transaction is retried due to contention. Move both the
logEntry call and sendEmail call outside of the transaction to ensure they
execute only once after the transaction successfully completes. Restore the
previous pattern by setting flags or storing alert data during the transaction
(inside the callback), then executing the actual side effects after the
transaction commits based on those flags, similar to how shouldAlert and
alertData were used before.
In `@cloud-functions/seed-events.js`:
- Around line 26-29: The seeded events are storing dates as ISO string formats
using toISOString(), but they should use Firestore-native timestamp types to
match production behavior and prevent type mismatches in tests. Replace the
toISOString() calls for startAt, endAt, and createdAt fields with Firestore's
native timestamp constructor (likely admin.firestore.Timestamp.fromDate() or
admin.firestore.Timestamp.now()), passing the Date objects directly instead of
converting them to strings. Apply this change to all instances mentioned in the
comment, including the ones around lines 49-52 as well.
In `@scripts/seed-early-bird-event.mjs`:
- Around line 46-50: The date fields createdAt, startAt, and endAt are currently
being stored as ISO string representations by calling toISOString() on the date
objects. Instead, these fields should use Firestore-native Timestamp objects for
proper emulator parity with production. Replace the toISOString() calls for
createdAt, startAt, and endAt with Firestore's Timestamp class (typically
imported from the firebase-admin SDK) to create proper temporal values that
Firestore can recognize as native date types.
---
Outside diff comments:
In `@app/src/components/CustomTabBar.js`:
- Around line 70-80: The TouchableOpacity component wrapping the tab buttons at
line 70 and the similar component at lines 127-131 has an effective touch target
size of 36x36 pixels, which is too small for reliable touch interaction. Add the
hitSlop prop to the TouchableOpacity component to expand the tappable area in
all directions (top, bottom, left, right) while keeping the visual size
unchanged. This will make the tab buttons easier to tap for users without
affecting the UI appearance.
---
Nitpick comments:
In `@cloud-functions/lib/branchReport.js`:
- Around line 286-290: The current code in the statsByBranch block only
increments attendance when a stats entry already exists for the branch. This
means check-ins from users whose branch doesn't have a registered stats entry
are silently dropped. To fix this, either create a new stats entry for
unregistered branches (following the same structure with attendance initialized
to 1 and events as a Set containing eventDoc.id) in an else clause after the if
(stats) block, or add a log statement to track when check-ins from unregistered
branches are skipped. This will either ensure all legitimate attendance is
counted or provide visibility into why some check-ins are being ignored.
In `@cloud-functions/seed-events.js`:
- Around line 65-67: The current implementation in the mockEvents.forEach loop
uses db.collection('events').doc() which generates random document IDs on every
execution, causing duplicate documents and unreliable emulator state. Replace
the random ID generation by either using fixed/deterministic IDs derived from
the event data (such as a unique property or hash) or implementing an upsert
strategy. Ensure the ref variable is assigned a consistent ID before calling
batch.set(ref, event) so that repeated seed runs maintain stable,
non-duplicative data.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 417e6cc1-0356-42ed-bdc7-3e37b374088d
⛔ Files ignored due to path filters (3)
app/package-lock.jsonis excluded by!**/package-lock.jsonapp/public/favicon.icois excluded by!**/*.icocloud-functions/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (22)
app/App.jsapp/package.jsonapp/src/components/CustomTabBar.jsapp/src/components/EventCard.jsapp/src/components/__tests__/EventCard.test.jsapp/src/screens/EventDetail.jsapp/src/screens/UserFeed.jscloud-functions/lib/attendanceStreak.jscloud-functions/lib/auditLog.jscloud-functions/lib/backfillEventAnalytics.jscloud-functions/lib/branchReport.jscloud-functions/lib/clubReputation.jscloud-functions/lib/computeShowUpRatios.jscloud-functions/lib/dedicatedStudentCertificate.jscloud-functions/lib/inactiveUsers.jscloud-functions/lib/logger.jscloud-functions/lib/onEventDelete.jscloud-functions/lib/permanentCleanup.jscloud-functions/lib/utils/rateLimiter.jscloud-functions/package.jsoncloud-functions/seed-events.jsscripts/seed-early-bird-event.mjs
💤 Files with no reviewable changes (1)
- cloud-functions/lib/inactiveUsers.js
…lback to npm install on cross-platform lockfile discrepancy
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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 @.github/workflows/ci.yml:
- Around line 72-76: Replace the two instances of `npm install --no-audit
--no-fund` with `npm ci --no-audit --no-fund` in the GitHub Actions workflow.
The first occurrence is in the general install dependencies step and the second
is in the cloud-functions install dependencies step (working-directory:
./cloud-functions). Using `npm ci` instead of `npm install` ensures that the
exact versions specified in the lockfile are used, providing deterministic and
reproducible builds in the CI environment.
In @.github/workflows/pr-validation.yml:
- Around line 7-13: The workflow currently lacks explicit token permission
definitions, relying on default scopes which is a security risk. Add a
permissions section at the workflow level (before or after the jobs key) to
explicitly define minimal required permissions. For validation-only jobs like
validate-functions, set permissions to read-only by adding permissions with
contents set to read, or use permissions.read-only set to true. Apply the same
minimal permission settings to the other validation job mentioned in the comment
as well.
- Around line 21-23: Replace the npm install command with npm ci in the PR
validation workflow file for improved reproducibility and lockfile consistency.
Specifically, change the run command on line 21 from using npm install with the
--no-audit and --no-fund flags to simply use npm ci, which does not require or
support these flags. Apply the same change to line 38 as mentioned in the
comment review to ensure all PR validation jobs use npm ci consistently.
- Around line 14-20: Replace the unpinned version tags in the GitHub Actions
with immutable commit SHAs for both validate-cloud-functions and validate-app
jobs. In the validate-cloud-functions job, change actions/checkout@v6 to
actions/checkout@<commit-sha> and actions/setup-node@v6 to
actions/setup-node@<commit-sha>. Add persist-credentials: false to the checkout
action configuration. Repeat the same pinning and persist-credentials addition
for the validate-app job at lines 31 and 33. Use the appropriate commit SHAs for
each action version (these can be looked up from the GitHub Actions marketplace
for the specific versions).
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: a6591490-bbff-414f-8970-a37feeb00890
⛔ Files ignored due to path filters (3)
app/package-lock.jsonis excluded by!**/package-lock.jsoncloud-functions/package-lock.jsonis excluded by!**/package-lock.jsonpackage-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (5)
.github/workflows/ci.yml.github/workflows/pr-validation.ymlapp/src/components/CustomTabBar.jsapp/src/screens/UserFeed.jscloud-functions/seed-events.js
💤 Files with no reviewable changes (1)
- app/src/screens/UserFeed.js
🚧 Files skipped from review as they are similar to previous changes (1)
- cloud-functions/seed-events.js
Owner
|
resolev the coderabbit suggestions and the issues flagged by sonar |
Contributor
Author
|
I have addressed the CodeRabbit suggestions and resolved the issues reported by Sonar. The changes have been pushed and all checks are passing. Kindly review the updated PR. Thank you. |
Owner
|
there are still 4 left |
|
Contributor
Author
|
I have addressed the CodeRabbit suggestions and resolved the issues reported by Sonar. all checks are passing. Kindly review the updated PR.Thank you. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
This PR implements three major frontend improvements to the Uni-Event platform:
@react-navigation/stackto@react-navigation/native-stackand integrated ReanimatedsharedTransitionTagfor event cover images. When a user taps an event card in the feed, the banner image smoothly scales ("grows") into the header of the details screen. Back navigation smoothly scales it down ("shrinks") back into place.useWindowDimensionshook in UserFeed.js to group events dynamically into multi-column rows. The feed shows 1 column on mobile (<768px), 2 columns on tablet (>=768px), and 3 columns on desktop/web (>=1024px) with robust flexbox spacing. We also moved recommended events into the main list directly and styled a yellow⭐ TOP PICKbadge directly on those cards.maxWidth: 500layout constraint on desktop. We reduced vertical padding, item container dimensions, icon sizing (size 22), and floating distance (bottom: 12) to make it look exceptionally slim and premium. Added dynamic dark mode support for Android platforms.Fixes # 86
Type of change
How Has This Been Tested?
EventCard.test.jswhich uses native stack mocks). Verified all 146 unit tests passed successfully.⭐ TOP PICKbadge rendered inside recommended cards, and that the floating bottom tab bar stayed slim, centered, and fully functional.Checklist:
Summary by CodeRabbit