Fix UI bugs #375
Fix UI bugs #375
Conversation
WalkthroughAdds a cache-backed accessor getCoreOrgRoles in AuthGuard (cache duration reduced to 30 minutes); updates OrgInfo to fetch core org roles explicitly and await both role sources; syncs semester selection to the URL in RoomRequestLanding; introduces RecursiveRecord typing and a new orgInfo e2e test; updates Makefile to support Terraform upgrade and pins AWS provider to 6.19.0 in qa/prod Terraform configs and lockfiles. Changes
Sequence DiagramssequenceDiagram
participant User
participant OrgInfoPage as OrgInfo Page
participant AuthGuard
participant Cache
User->>OrgInfoPage: Mount
par Fetch roles in parallel
OrgInfoPage->>AuthGuard: getUserRoles()
AuthGuard->>Cache: Read appRoles cache (app authCheckRoute)
Cache-->>AuthGuard: appRoles | miss
AuthGuard-->>OrgInfoPage: appRoles[] | null
and
OrgInfoPage->>AuthGuard: getCoreOrgRoles()
AuthGuard->>Cache: Read orgRoles cache (core authCheckRoute)
Cache-->>AuthGuard: orgRoles | miss
AuthGuard-->>OrgInfoPage: OrgRoleDefinition[] | null
end
alt both non-null
OrgInfoPage->>OrgInfoPage: Determine access (ALL_ORG_MANAGER / LEAD filter)
OrgInfoPage->>User: Render authorized view
else any null
OrgInfoPage->>User: Halt / show authorization error
end
sequenceDiagram
participant User
participant RoomRequest as RoomRequestLanding
participant Router as Browser URL
User->>RoomRequest: Mount
RoomRequest->>Router: read ?semester
alt valid semester in URL
Router-->>RoomRequest: semester value
RoomRequest->>RoomRequest: set semester state from URL
else no/invalid semester
Router-->>RoomRequest: (none)
RoomRequest->>RoomRequest: set semester state to default next semester
RoomRequest->>Router: update ?semester=default
end
User->>RoomRequest: change semester selection
RoomRequest->>RoomRequest: setSemester(new)
RoomRequest->>Router: update ?semester=new
Router-->>User: URL updated
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~30 minutes
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro Cache: Disabled due to data retention organization setting Knowledge base: Disabled due to data retention organization setting 📒 Files selected for processing (5)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
🔇 Additional comments (6)
Comment |
💰 Infracost reportMonthly estimate generatedThis comment will be updated when code changes. |
![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
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
📒 Files selected for processing (3)
src/ui/components/AuthGuard/index.tsx(2 hunks)src/ui/pages/organization/OrgInfo.page.tsx(2 hunks)src/ui/pages/roomRequest/RoomRequestLanding.page.tsx(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Build Application
- GitHub Check: Run Unit Tests
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
tests/e2e/base.ts (1)
7-9: Consider replacinganywithunknownfor better type safety.The
anytype bypasses TypeScript's type checking entirely. For test data structures where the shape may vary,unknownprovides safety by requiring explicit type narrowing at usage sites.Apply this diff to improve type safety:
-export interface RecursiveRecord - extends Record<string, any | RecursiveRecord> {} +export interface RecursiveRecord + extends Record<string, unknown> {}Alternatively, if you need explicit recursive nesting support:
-export interface RecursiveRecord - extends Record<string, any | RecursiveRecord> {} +export interface RecursiveRecord { + [key: string]: unknown | RecursiveRecord | RecursiveRecord[]; +}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
📒 Files selected for processing (2)
tests/e2e/base.ts(2 hunks)tests/e2e/orgInfo.spec.ts(1 hunks)
🧰 Additional context used
🪛 ESLint
tests/e2e/orgInfo.spec.ts
[error] 1-1: Resolve error: EACCES: permission denied, open '/dXWFallTck'
at Object.writeFileSync (node:fs:2409:20)
at l (/home/jailuser/git/node_modules/get-tsconfig/dist/index.cjs:7:13685)
at createFilesMatcher (/home/jailuser/git/node_modules/get-tsconfig/dist/index.cjs:7:14437)
at Object.resolve (/home/jailuser/git/node_modules/eslint-import-resolver-typescript/lib/index.cjs:298:107)
at withResolver (/home/jailuser/git/node_modules/eslint-module-utils/resolve.js:180:23)
at fullResolve (/home/jailuser/git/node_modules/eslint-module-utils/resolve.js:201:22)
at relative (/home/jailuser/git/node_modules/eslint-module-utils/resolve.js:217:10)
at resolve (/home/jailuser/git/node_modules/eslint-module-utils/resolve.js:233:12)
at checkFileExtension (/home/jailuser/git/node_modules/eslint-plugin-import/lib/rules/extensions.js:205:53)
at checkSourceValue (/home/jailuser/git/node_modules/eslint-module-utils/moduleVisitor.js:32:5)
(import/extensions)
[error] 2-2: Unexpected use of file extension "js" for "./base.js"
(import/extensions)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Run Unit Tests
- GitHub Check: Build Application
🔇 Additional comments (4)
tests/e2e/orgInfo.spec.ts (4)
6-28: LGTM!The test setup, authentication, navigation, and form population logic is well-structured. The use of ISO timestamps for uniqueness is a good practice for parallel test execution and avoiding data conflicts.
29-43: LGTM!The conditional logic for handling the "Other" link demonstrates good test design—checking for existing state before modification ensures the test is idempotent and won't fail on repeated runs.
55-70: LGTM!The API verification logic is thorough and correctly validates that the UI changes are persisted. The use of
RecursiveRecordtype assertions aligns with the type changes inbase.ts, and the checks for description, website, and links are comprehensive.
52-54: Let me re-run the verification with a different approach that doesn't require sha256sum:Verify whether the
datequery parameter actually busts cache, or remove it if unnecessary.CloudFront can cache query parameters separately, but whether this works depends on CloudFront's configuration. If the API ignores unrecognized query parameters, CloudFront may still cache them separately (though the origin returns identical objects). However, S3 origins ignore query parameters entirely, so they don't work as cache busters there.
Since this test calls a backend API endpoint (not S3), cache busting could work if CloudFront is configured to forward and cache based on query parameters. However, without confirmation that the
dateparameter actually serves a purpose in this test—or that the API uses it—consider whether this parameter is necessary or if it's redundant overhead.
| "https://core.aws.qa.acmuiuc.org/api/v1/events?upcomingOnly=true", | ||
| ); | ||
| return (await data.json()) as Record<string, string>[]; | ||
| return (await data.json()) as RecursiveRecord[]; |
There was a problem hiding this comment.
🧩 Analysis chain
Verify that API responses contain nested structures.
The return type was broadened from Record<string, string>[] to RecursiveRecord[], allowing nested objects. Ensure this change accurately reflects the actual API response structure.
Run the following script to inspect the actual API response shape:
Also applies to: 82-82
🏁 Script executed:
#!/bin/bash
# Fetch sample API responses to verify nested structure
echo "=== Upcoming Events Response ==="
curl -s "https://core.aws.qa.acmuiuc.org/api/v1/events?upcomingOnly=true" | jq 'if type == "array" then .[0] else . end | .. | select(type == "object" or type == "array") | type' | sort | uniq -c
echo -e "\n=== All Events Response ==="
curl -s "https://core.aws.qa.acmuiuc.org/api/v1/events" | jq 'if type == "array" then .[0] else . end | .. | select(type == "object" or type == "array") | type' | sort | uniq -cLength of output: 95
I need to inspect the actual API response structure more directly. Let me gather additional information:
Now let me inspect the codebase to find the RecursiveRecord definition and verify the actual API response:
---
🏁 Script executed:
```shell
#!/bin/bash
# Find RecursiveRecord type definition
rg -n "RecursiveRecord" --type ts --type tsx -B2 -A5
Length of output: 27
🏁 Script executed:
#!/bin/bash
# Get actual API response to inspect structure
echo "=== Upcoming Events Response (pretty-printed) ==="
curl -s "https://core.aws.qa.acmuiuc.org/api/v1/events?upcomingOnly=true" | jq '.[0]' 2>/dev/null | head -50
echo -e "\n=== Response type info ==="
curl -s "https://core.aws.qa.acmuiuc.org/api/v1/events?upcomingOnly=true" | jq 'type'Length of output: 361
Let me search for the RecursiveRecord definition and check the test file context:
🏁 Script executed:
#!/bin/bash
# Find RecursiveRecord type definition (fix file type error)
rg -n "RecursiveRecord" --type ts -B2 -A5Length of output: 2826
🏁 Script executed:
#!/bin/bash
# Check the test file for context around the type change
cat tests/e2e/base.ts | head -100Length of output: 2793
Revert type to more specific definition for events responses.
The events API responses are flat structures containing only primitive values (strings, booleans), not nested objects. The change to RecursiveRecord[] is overly broad and reduces type safety. Use a more specific type like Record<string, string | boolean>[] that accurately reflects the actual API response structure.
🤖 Prompt for AI Agents
In tests/e2e/base.ts around line 77, the return type is overly broad
(RecursiveRecord[]) for events API responses which are flat primitives; change
the type to a specific array of records containing only string or boolean values
by replacing RecursiveRecord[] with Record<string, string | boolean>[] so the
function returns (await data.json()) as Record<string, string | boolean>[] and
restores stricter type safety.
| @@ -0,0 +1,72 @@ | |||
| import { expect } from "@playwright/test"; | |||
| import { RecursiveRecord, test } from "./base.js"; | |||
| import { describe } from "node:test"; | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Avoid mixing node:test with Playwright's test framework.
Importing describe from node:test while using Playwright's test fixture can lead to unexpected behavior. Playwright provides its own test organization primitives.
Apply this diff to use Playwright's built-in test organization:
-import { expect } from "@playwright/test";
+import { expect, test } from "@playwright/test";
-import { RecursiveRecord, test } from "./base.js";
+import { RecursiveRecord } from "./base.js";
-import { describe } from "node:test";
-describe("Organization Info Tests", () => {
- test("A user can update org metadata", async ({ page, becomeUser }) => {
+test.describe("Organization Info Tests", () => {
+ test("A user can update org metadata", async ({ page, becomeUser }) => {Or simply remove describe if grouping is not essential:
-import { expect } from "@playwright/test";
+import { expect, test } from "@playwright/test";
-import { RecursiveRecord, test } from "./base.js";
+import { RecursiveRecord } from "./base.js";
-import { describe } from "node:test";
-describe("Organization Info Tests", () => {
- test("A user can update org metadata", async ({ page, becomeUser }) => {
+test("A user can update org metadata", async ({ page, becomeUser }) => {Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In tests/e2e/orgInfo.spec.ts around line 3, the file imports describe from
node:test which conflicts with Playwright's test framework; remove the node:test
import and either use Playwright's test.describe (i.e., import { test } from
'@playwright/test' and call test.describe(...)) or import describe from
'@playwright/test' if you prefer that API, ensuring all test groups use
Playwright primitives consistently.
Summary by CodeRabbit
New Features
Performance
Bug Fixes
Tests
Chores