appview/db: split star subjects #824

+118 -139

12 changed files

Diff round #3

expand all

unified split

appview

db.go

repos.go

star.go

timeline.go

ingester.go

models

star.go

timeline.go

notify

db.go

posthog

notifier.go

pages

templates

timeline

fragments

timeline.html

state

profile.go

star.go

+39 -2

appview/db/db.go

··· 569 -- indexes for better performance 570 create index if not exists idx_notifications_recipient_created on notifications(recipient_did, created desc); 571 create index if not exists idx_notifications_recipient_read on notifications(recipient_did, read); 572 `) 573 if err != nil { 574 return nil, err ··· 1126 return err 1127 }) 1128 1129 + // remove the foreign key constraints from stars. 1130 + runMigration(conn, logger, "generalize-stars-subject", func(tx *sql.Tx) error { 1131 + _, err := tx.Exec(` 1132 + create table stars_new ( 1133 + id integer primary key autoincrement, 1134 + did text not null, 1135 + rkey text not null, 1136 + 1137 + subject_at text not null, 1138 + 1139 + created text not null default (strftime('%Y-%m-%dT%H:%M:%SZ', 'now')), 1140 + unique(did, rkey), 1141 + unique(did, subject_at) 1142 + ); 1143 + 1144 + insert into stars_new ( 1145 + id, 1146 + did, 1147 + rkey, 1148 + subject_at, 1149 + created 1150 + ) 1151 + select 1152 + id, 1153 + starred_by_did, 1154 + rkey, 1155 + repo_at, 1156 + created 1157 + from stars; 1158 + 1159 + drop table stars; 1160 + alter table stars_new rename to stars; 1161 + 1162 + create index if not exists idx_stars_created on stars(created); 1163 + create index if not exists idx_stars_subject_at_created on stars(subject_at, created); 1164 + `) 1165 + return err 1166 + }) 1167 + 1168 return &DB{ 1169 db, 1170 logger,

+3 -3

appview/db/repos.go

··· 208 209 starCountQuery := fmt.Sprintf( 210 `select 211 - repo_at, count(1) 212 from stars 213 - where repo_at in (%s) 214 - group by repo_at`, 215 inClause, 216 ) 217 rows, err = e.Query(starCountQuery, args...)

··· 208 209 starCountQuery := fmt.Sprintf( 210 `select 211 + subject_at, count(1) 212 from stars 213 + where subject_at in (%s) 214 + group by subject_at`, 215 inClause, 216 ) 217 rows, err = e.Query(starCountQuery, args...)

+39 -99

appview/db/star.go

··· 14 ) 15 16 func AddStar(e Execer, star *models.Star) error { 17 - query := `insert or ignore into stars (starred_by_did, repo_at, rkey) values (?, ?, ?)` 18 _, err := e.Exec( 19 query, 20 - star.StarredByDid, 21 star.RepoAt.String(), 22 star.Rkey, 23 ) ··· 25 } 26 27 // Get a star record 28 - func GetStar(e Execer, starredByDid string, repoAt syntax.ATURI) (*models.Star, error) { 29 query := ` 30 - select starred_by_did, repo_at, created, rkey 31 from stars 32 - where starred_by_did = ? and repo_at = ?` 33 - row := e.QueryRow(query, starredByDid, repoAt) 34 35 var star models.Star 36 var created string 37 - err := row.Scan(&star.StarredByDid, &star.RepoAt, &created, &star.Rkey) 38 if err != nil { 39 return nil, err 40 } ··· 51 } 52 53 // Remove a star 54 - func DeleteStar(e Execer, starredByDid string, repoAt syntax.ATURI) error { 55 - _, err := e.Exec(`delete from stars where starred_by_did = ? and repo_at = ?`, starredByDid, repoAt) 56 return err 57 } 58 59 // Remove a star 60 - func DeleteStarByRkey(e Execer, starredByDid string, rkey string) error { 61 - _, err := e.Exec(`delete from stars where starred_by_did = ? and rkey = ?`, starredByDid, rkey) 62 return err 63 } 64 65 - func GetStarCount(e Execer, repoAt syntax.ATURI) (int, error) { 66 stars := 0 67 err := e.QueryRow( 68 - `select count(starred_by_did) from stars where repo_at = ?`, repoAt).Scan(&stars) 69 if err != nil { 70 return 0, err 71 } ··· 89 } 90 91 query := fmt.Sprintf(` 92 - SELECT repo_at 93 FROM stars 94 - WHERE starred_by_did = ? AND repo_at IN (%s) 95 `, strings.Join(placeholders, ",")) 96 97 rows, err := e.Query(query, args...) ··· 118 return result, nil 119 } 120 121 - func GetStarStatus(e Execer, userDid string, repoAt syntax.ATURI) bool { 122 - statuses, err := getStarStatuses(e, userDid, []syntax.ATURI{repoAt}) 123 if err != nil { 124 return false 125 } 126 - return statuses[repoAt.String()] 127 } 128 129 // GetStarStatuses returns a map of repo URIs to star status for a given user 130 - func GetStarStatuses(e Execer, userDid string, repoAts []syntax.ATURI) (map[string]bool, error) { 131 - return getStarStatuses(e, userDid, repoAts) 132 } 133 - func GetStars(e Execer, limit int, filters ...filter) ([]models.Star, error) { 134 var conditions []string 135 var args []any 136 for _, filter := range filters { ··· 149 } 150 151 repoQuery := fmt.Sprintf( 152 - `select starred_by_did, repo_at, created, rkey 153 from stars 154 %s 155 order by created desc ··· 166 for rows.Next() { 167 var star models.Star 168 var created string 169 - err := rows.Scan(&star.StarredByDid, &star.RepoAt, &created, &star.Rkey) 170 if err != nil { 171 return nil, err 172 } ··· 197 return nil, err 198 } 199 200 for _, r := range repos { 201 if stars, ok := starMap[string(r.RepoAt())]; ok { 202 - for i := range stars { 203 - stars[i].Repo = &r 204 } 205 } 206 } 207 208 - var stars []models.Star 209 - for _, s := range starMap { 210 - stars = append(stars, s...) 211 - } 212 - 213 - slices.SortFunc(stars, func(a, b models.Star) int { 214 if a.Created.After(b.Created) { 215 return -1 216 } ··· 220 return 0 221 }) 222 223 - return stars, nil 224 } 225 226 func CountStars(e Execer, filters ...filter) (int64, error) { ··· 247 return count, nil 248 } 249 250 - func GetAllStars(e Execer, limit int) ([]models.Star, error) { 251 - var stars []models.Star 252 - 253 - rows, err := e.Query(` 254 - select 255 - s.starred_by_did, 256 - s.repo_at, 257 - s.rkey, 258 - s.created, 259 - r.did, 260 - r.name, 261 - r.knot, 262 - r.rkey, 263 - r.created 264 - from stars s 265 - join repos r on s.repo_at = r.at_uri 266 - `) 267 - 268 - if err != nil { 269 - return nil, err 270 - } 271 - defer rows.Close() 272 - 273 - for rows.Next() { 274 - var star models.Star 275 - var repo models.Repo 276 - var starCreatedAt, repoCreatedAt string 277 - 278 - if err := rows.Scan( 279 - &star.StarredByDid, 280 - &star.RepoAt, 281 - &star.Rkey, 282 - &starCreatedAt, 283 - &repo.Did, 284 - &repo.Name, 285 - &repo.Knot, 286 - &repo.Rkey, 287 - &repoCreatedAt, 288 - ); err != nil { 289 - return nil, err 290 - } 291 - 292 - star.Created, err = time.Parse(time.RFC3339, starCreatedAt) 293 - if err != nil { 294 - star.Created = time.Now() 295 - } 296 - repo.Created, err = time.Parse(time.RFC3339, repoCreatedAt) 297 - if err != nil { 298 - repo.Created = time.Now() 299 - } 300 - star.Repo = &repo 301 - 302 - stars = append(stars, star) 303 - } 304 - 305 - if err := rows.Err(); err != nil { 306 - return nil, err 307 - } 308 - 309 - return stars, nil 310 - } 311 - 312 // GetTopStarredReposLastWeek returns the top 8 most starred repositories from the last week 313 func GetTopStarredReposLastWeek(e Execer) ([]models.Repo, error) { 314 // first, get the top repo URIs by star count from the last week 315 query := ` 316 with recent_starred_repos as ( 317 - select distinct repo_at 318 from stars 319 where created >= datetime('now', '-7 days') 320 ), 321 repo_star_counts as ( 322 select 323 - s.repo_at, 324 count(*) as stars_gained_last_week 325 from stars s 326 - join recent_starred_repos rsr on s.repo_at = rsr.repo_at 327 where s.created >= datetime('now', '-7 days') 328 - group by s.repo_at 329 ) 330 - select rsc.repo_at 331 from repo_star_counts rsc 332 order by rsc.stars_gained_last_week desc 333 limit 8

··· 14 ) 15 16 func AddStar(e Execer, star *models.Star) error { 17 + query := `insert or ignore into stars (did, subject_at, rkey) values (?, ?, ?)` 18 _, err := e.Exec( 19 query, 20 + star.Did, 21 star.RepoAt.String(), 22 star.Rkey, 23 ) ··· 25 } 26 27 // Get a star record 28 + func GetStar(e Execer, did string, subjectAt syntax.ATURI) (*models.Star, error) { 29 query := ` 30 + select did, subject_at, created, rkey 31 from stars 32 + where did = ? and subject_at = ?` 33 + row := e.QueryRow(query, did, subjectAt) 34 35 var star models.Star 36 var created string 37 + err := row.Scan(&star.Did, &star.RepoAt, &created, &star.Rkey) 38 if err != nil { 39 return nil, err 40 } ··· 51 } 52 53 // Remove a star 54 + func DeleteStar(e Execer, did string, subjectAt syntax.ATURI) error { 55 + _, err := e.Exec(`delete from stars where did = ? and subject_at = ?`, did, subjectAt) 56 return err 57 } 58 59 // Remove a star 60 + func DeleteStarByRkey(e Execer, did string, rkey string) error { 61 + _, err := e.Exec(`delete from stars where did = ? and rkey = ?`, did, rkey) 62 return err 63 } 64 65 + func GetStarCount(e Execer, subjectAt syntax.ATURI) (int, error) { 66 stars := 0 67 err := e.QueryRow( 68 + `select count(did) from stars where subject_at = ?`, subjectAt).Scan(&stars) 69 if err != nil { 70 return 0, err 71 } ··· 89 } 90 91 query := fmt.Sprintf(` 92 + SELECT subject_at 93 FROM stars 94 + WHERE did = ? AND subject_at IN (%s) 95 `, strings.Join(placeholders, ",")) 96 97 rows, err := e.Query(query, args...) ··· 118 return result, nil 119 } 120 121 + func GetStarStatus(e Execer, userDid string, subjectAt syntax.ATURI) bool { 122 + statuses, err := getStarStatuses(e, userDid, []syntax.ATURI{subjectAt}) 123 if err != nil { 124 return false 125 } 126 + return statuses[subjectAt.String()] 127 } 128 129 // GetStarStatuses returns a map of repo URIs to star status for a given user 130 + func GetStarStatuses(e Execer, userDid string, subjectAts []syntax.ATURI) (map[string]bool, error) { 131 + return getStarStatuses(e, userDid, subjectAts) 132 } 133 + 134 + // GetRepoStars return a list of stars each holding target repository. 135 + // If there isn't known repo with starred at-uri, those stars will be ignored. 136 + func GetRepoStars(e Execer, limit int, filters ...filter) ([]models.RepoStar, error) { 137 var conditions []string 138 var args []any 139 for _, filter := range filters { ··· 152 } 153 154 repoQuery := fmt.Sprintf( 155 + `select did, subject_at, created, rkey 156 from stars 157 %s 158 order by created desc ··· 169 for rows.Next() { 170 var star models.Star 171 var created string 172 + err := rows.Scan(&star.Did, &star.RepoAt, &created, &star.Rkey) 173 if err != nil { 174 return nil, err 175 } ··· 200 return nil, err 201 } 202 203 + var repoStars []models.RepoStar 204 for _, r := range repos { 205 if stars, ok := starMap[string(r.RepoAt())]; ok { 206 + for _, star := range stars { 207 + repoStars = append(repoStars, models.RepoStar{ 208 + Star: star, 209 + Repo: &r, 210 + }) 211 } 212 } 213 } 214 215 + slices.SortFunc(repoStars, func(a, b models.RepoStar) int { 216 if a.Created.After(b.Created) { 217 return -1 218 } ··· 222 return 0 223 }) 224 225 + return repoStars, nil 226 } 227 228 func CountStars(e Execer, filters ...filter) (int64, error) { ··· 249 return count, nil 250 } 251 252 // GetTopStarredReposLastWeek returns the top 8 most starred repositories from the last week 253 func GetTopStarredReposLastWeek(e Execer) ([]models.Repo, error) { 254 // first, get the top repo URIs by star count from the last week 255 query := ` 256 with recent_starred_repos as ( 257 + select distinct subject_at 258 from stars 259 where created >= datetime('now', '-7 days') 260 ), 261 repo_star_counts as ( 262 select 263 + s.subject_at, 264 count(*) as stars_gained_last_week 265 from stars s 266 + join recent_starred_repos rsr on s.subject_at = rsr.subject_at 267 where s.created >= datetime('now', '-7 days') 268 + group by s.subject_at 269 ) 270 + select rsc.subject_at 271 from repo_star_counts rsc 272 order by rsc.stars_gained_last_week desc 273 limit 8

+3 -13

appview/db/timeline.go

··· 146 func getTimelineStars(e Execer, limit int, loggedInUserDid string, userIsFollowing []string) ([]models.TimelineEvent, error) { 147 filters := make([]filter, 0) 148 if userIsFollowing != nil { 149 - filters = append(filters, FilterIn("starred_by_did", userIsFollowing)) 150 } 151 152 - stars, err := GetStars(e, limit, filters...) 153 if err != nil { 154 return nil, err 155 } 156 157 - // filter star records without a repo 158 - n := 0 159 - for _, s := range stars { 160 - if s.Repo != nil { 161 - stars[n] = s 162 - n++ 163 - } 164 - } 165 - stars = stars[:n] 166 - 167 var repos []models.Repo 168 for _, s := range stars { 169 repos = append(repos, *s.Repo) ··· 179 isStarred, starCount := getRepoStarInfo(s.Repo, starStatuses) 180 181 events = append(events, models.TimelineEvent{ 182 - Star: &s, 183 EventAt: s.Created, 184 IsStarred: isStarred, 185 StarCount: starCount,

··· 146 func getTimelineStars(e Execer, limit int, loggedInUserDid string, userIsFollowing []string) ([]models.TimelineEvent, error) { 147 filters := make([]filter, 0) 148 if userIsFollowing != nil { 149 + filters = append(filters, FilterIn("did", userIsFollowing)) 150 } 151 152 + stars, err := GetRepoStars(e, limit, filters...) 153 if err != nil { 154 return nil, err 155 } 156 157 var repos []models.Repo 158 for _, s := range stars { 159 repos = append(repos, *s.Repo) ··· 169 isStarred, starCount := getRepoStarInfo(s.Repo, starStatuses) 170 171 events = append(events, models.TimelineEvent{ 172 + RepoStar: &s, 173 EventAt: s.Created, 174 IsStarred: isStarred, 175 StarCount: starCount,

+3 -3

appview/ingester.go

··· 121 return err 122 } 123 err = db.AddStar(i.Db, &models.Star{ 124 - StarredByDid: did, 125 - RepoAt: subjectUri, 126 - Rkey: e.Commit.RKey, 127 }) 128 case jmodels.CommitOperationDelete: 129 err = db.DeleteStarByRkey(i.Db, did, e.Commit.RKey)

··· 121 return err 122 } 123 err = db.AddStar(i.Db, &models.Star{ 124 + Did: did, 125 + RepoAt: subjectUri, 126 + Rkey: e.Commit.RKey, 127 }) 128 case jmodels.CommitOperationDelete: 129 err = db.DeleteStarByRkey(i.Db, did, e.Commit.RKey)

+14 -5

appview/models/star.go

··· 7 ) 8 9 type Star struct { 10 - StarredByDid string 11 - RepoAt syntax.ATURI 12 - Created time.Time 13 - Rkey string 14 15 - // optionally, populate this when querying for reverse mappings 16 Repo *Repo 17 }

··· 7 ) 8 9 type Star struct { 10 + Did string 11 + RepoAt syntax.ATURI 12 + Created time.Time 13 + Rkey string 14 + } 15 16 + // RepoStar is used for reverse mapping to repos 17 + type RepoStar struct { 18 + Star 19 Repo *Repo 20 } 21 + 22 + // StringStar is used for reverse mapping to strings 23 + type StringStar struct { 24 + Star 25 + String *String 26 + }

+1 -1

appview/models/timeline.go

··· 5 type TimelineEvent struct { 6 *Repo 7 *Follow 8 - *Star 9 10 EventAt time.Time 11

··· 5 type TimelineEvent struct { 6 *Repo 7 *Follow 8 + *RepoStar 9 10 EventAt time.Time 11

+6 -1

appview/notify/db/db.go

··· 7 "slices" 8 9 "github.com/bluesky-social/indigo/atproto/syntax" 10 "tangled.org/core/appview/db" 11 "tangled.org/core/appview/models" 12 "tangled.org/core/appview/notify" ··· 36 } 37 38 func (n *databaseNotifier) NewStar(ctx context.Context, star *models.Star) { 39 var err error 40 repo, err := db.GetRepo(n.db, db.FilterEq("at_uri", string(star.RepoAt))) 41 if err != nil { ··· 43 return 44 } 45 46 - actorDid := syntax.DID(star.StarredByDid) 47 recipients := []syntax.DID{syntax.DID(repo.Did)} 48 eventType := models.NotificationTypeRepoStarred 49 entityType := "repo"

··· 7 "slices" 8 9 "github.com/bluesky-social/indigo/atproto/syntax" 10 + "tangled.org/core/api/tangled" 11 "tangled.org/core/appview/db" 12 "tangled.org/core/appview/models" 13 "tangled.org/core/appview/notify" ··· 37 } 38 39 func (n *databaseNotifier) NewStar(ctx context.Context, star *models.Star) { 40 + if star.RepoAt.Collection().String() != tangled.RepoNSID { 41 + // skip string stars for now 42 + return 43 + } 44 var err error 45 repo, err := db.GetRepo(n.db, db.FilterEq("at_uri", string(star.RepoAt))) 46 if err != nil { ··· 48 return 49 } 50 51 + actorDid := syntax.DID(star.Did) 52 recipients := []syntax.DID{syntax.DID(repo.Did)} 53 eventType := models.NotificationTypeRepoStarred 54 entityType := "repo"

+2 -2

appview/notify/posthog/notifier.go

··· 37 38 func (n *posthogNotifier) NewStar(ctx context.Context, star *models.Star) { 39 err := n.client.Enqueue(posthog.Capture{ 40 - DistinctId: star.StarredByDid, 41 Event: "star", 42 Properties: posthog.Properties{"repo_at": star.RepoAt.String()}, 43 }) ··· 48 49 func (n *posthogNotifier) DeleteStar(ctx context.Context, star *models.Star) { 50 err := n.client.Enqueue(posthog.Capture{ 51 - DistinctId: star.StarredByDid, 52 Event: "unstar", 53 Properties: posthog.Properties{"repo_at": star.RepoAt.String()}, 54 })

··· 37 38 func (n *posthogNotifier) NewStar(ctx context.Context, star *models.Star) { 39 err := n.client.Enqueue(posthog.Capture{ 40 + DistinctId: star.Did, 41 Event: "star", 42 Properties: posthog.Properties{"repo_at": star.RepoAt.String()}, 43 }) ··· 48 49 func (n *posthogNotifier) DeleteStar(ctx context.Context, star *models.Star) { 50 err := n.client.Enqueue(posthog.Capture{ 51 + DistinctId: star.Did, 52 Event: "unstar", 53 Properties: posthog.Properties{"repo_at": star.RepoAt.String()}, 54 })

+2 -2

appview/pages/templates/timeline/fragments/timeline.html

··· 59 {{ define "timeline/fragments/starEvent" }} 60 {{ $root := index . 0 }} 61 {{ $event := index . 1 }} 62 - {{ $star := $event.Star }} 63 {{ with $star }} 64 - {{ $starrerHandle := resolve .StarredByDid }} 65 {{ $repoOwnerHandle := resolve .Repo.Did }} 66 <div class="pl-6 py-2 bg-white dark:bg-gray-800 text-gray-600 dark:text-gray-300 flex flex-wrap items-center gap-2 text-sm"> 67 {{ template "user/fragments/picHandleLink" $starrerHandle }}

··· 59 {{ define "timeline/fragments/starEvent" }} 60 {{ $root := index . 0 }} 61 {{ $event := index . 1 }} 62 + {{ $star := $event.RepoStar }} 63 {{ with $star }} 64 + {{ $starrerHandle := resolve .Did }} 65 {{ $repoOwnerHandle := resolve .Repo.Did }} 66 <div class="pl-6 py-2 bg-white dark:bg-gray-800 text-gray-600 dark:text-gray-300 flex flex-wrap items-center gap-2 text-sm"> 67 {{ template "user/fragments/picHandleLink" $starrerHandle }}

+3 -5

appview/state/profile.go

··· 66 return nil, fmt.Errorf("failed to get string count: %w", err) 67 } 68 69 - starredCount, err := db.CountStars(s.db, db.FilterEq("starred_by_did", did)) 70 if err != nil { 71 return nil, fmt.Errorf("failed to get starred repo count: %w", err) 72 } ··· 211 } 212 l = l.With("profileDid", profile.UserDid, "profileHandle", profile.UserHandle) 213 214 - stars, err := db.GetStars(s.db, 0, db.FilterEq("starred_by_did", profile.UserDid)) 215 if err != nil { 216 l.Error("failed to get stars", "err", err) 217 s.pages.Error500(w) ··· 219 } 220 var repos []models.Repo 221 for _, s := range stars { 222 - if s.Repo != nil { 223 - repos = append(repos, *s.Repo) 224 - } 225 } 226 227 err = s.pages.ProfileStarred(w, pages.ProfileStarredParams{

··· 66 return nil, fmt.Errorf("failed to get string count: %w", err) 67 } 68 69 + starredCount, err := db.CountStars(s.db, db.FilterEq("did", did)) 70 if err != nil { 71 return nil, fmt.Errorf("failed to get starred repo count: %w", err) 72 } ··· 211 } 212 l = l.With("profileDid", profile.UserDid, "profileHandle", profile.UserHandle) 213 214 + stars, err := db.GetRepoStars(s.db, 0, db.FilterEq("did", profile.UserDid)) 215 if err != nil { 216 l.Error("failed to get stars", "err", err) 217 s.pages.Error500(w) ··· 219 } 220 var repos []models.Repo 221 for _, s := range stars { 222 + repos = append(repos, *s.Repo) 223 } 224 225 err = s.pages.ProfileStarred(w, pages.ProfileStarredParams{

+3 -3

appview/state/star.go

··· 57 log.Println("created atproto record: ", resp.Uri) 58 59 star := &models.Star{ 60 - StarredByDid: currentUser.Did, 61 - RepoAt: subjectUri, 62 - Rkey: rkey, 63 } 64 65 err = db.AddStar(s.db, star)

··· 57 log.Println("created atproto record: ", resp.Uri) 58 59 star := &models.Star{ 60 + Did: currentUser.Did, 61 + RepoAt: subjectUri, 62 + Rkey: rkey, 63 } 64 65 err = db.AddStar(s.db, star)

History

5 rounds 3 comments

boltless.me submitted #4 3mo

diff interdiff

1 commit

expand

17e92c3c

appview/db: split star subjects

3/3 success

expand

expand 0 comments

pull request successfully merged

boltless.me submitted #3 3mo

interdiff

1 commit

expand

17e92c3c

appview/db: split star subjects

3/3 success

expand

expand 0 comments

boltless.me submitted #2 3mo

diff interdiff

1 commit

expand

c6167de1

appview/db: split star subjects

3/3 success

expand

expand 0 comments

boltless.me submitted #1 3mo

diff interdiff

1 commit

expand

10e3b844

appview/db: split star subjects

3/3 success

expand

expand 0 comments

boltless.me submitted #0 3mo

diff

1 commit

expand

fb8b603e

appview/db: split star subjects

1/3 failed, 2/3 success

expand

expand 3 comments

oppi.li 3mo

bit confusing that the repo_at col of the stars table can now refer to anything. can we rename that column too? to something more generic like target?

boltless.me 3mo

Actually that was my first intention, renaming repo_at to subject_at. I just didn't do that here because I was afraid to find all .repo_at usages in raw sql queries. And I thought that will also harder to review without tests.

Do you think it is worth renaming? If so, I can do that.

oppi.li 3mo

IIUC we'd only need to find repo_at for all queries that mention the "stars" table. searching for from stars might cover that. I'd recommend that we do fix this!